1614850 - Enabling layers.progressive-paint causes Firefox to crash
Closed Bug 1614850 Opened 5 years ago Closed 4 years ago

Enabling layers.progressive-paint causes Firefox to crash

Categories

(Core :: Graphics: Layers, defect, P5)

73 Branch
defect

Tracking

()

RESOLVED FIXED
88 Branch
Tracking Status
firefox73 --- wontfix
firefox74 --- wontfix
firefox75 --- wontfix
firefox88 --- fixed

People

(Reporter: eselstam, Assigned: 4gateftw, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [lang=c++])

Attachments

(2 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

  • Start Firefox with $ firefox --ProfileManager --safe-mode.
  • Create a new profile.
  • Enter about:config and bypass "I’ll be careful”.
  • Search for layers.progressive-paint.
  • Set aforementioned setting to true.

Actual results:

Firefox crashes with a SIGSEGV (segmentation fault). The crashing thread seems to be the compositor thread at mozilla::CrossProcessMutex::CrossProcessMutex.

The defect is present in Firefox 73.0 Stable, but has persisted since at least 72.0.2 Stable. If memory serves, the bug was introduced in 72.0.2, but this is not verified.

Expected results:

Firefox not crashing. Alternatively handling the case in a more graceful manner, eg. disabling layers.progressive-paint on next restart or when using safe mode.

If progressive painting is deprecated, the chosen action might be to ignore the option completely, or warn on its presence.

The test was run on Arch Linux running Linux 5.4.15 with proprietary Nvidia 440.44-15 drivers.

Worth noting is that since the setting now causes a crash, changing it means that it’s no longer possible to start Firefox without manually reverting prefs.js in the profile directory. Trying to start Firefox simply leads directly to a crash dialog.

Although this should mostly be of concern for those intrepid enough to change the setting, it does merit attention since profiles with this preference already enabled in previous versions now experience a browser that won’t start. Personally I had the preference on for many years without issue, likely since somewhere around 2015-2016.

I have further tested the test scenario above on the following setups:

Firefox Version Operating System Status
73.0 Release Arch Linux + Nvidia Bug present
73.0 Release macOS 10.14.6 + Intel Bug present
72.0.2 Release Arch Linux + Nvidia Bug present
72.0.1 Release Arch Linux + Nvidia Bug present
71.0 Release Arch Linux + Nvidia Bug not present

In summary, the regression seems to have occurred somewhere between 71.0 and 72.0.1 on the Release channel.

Hi Selis,

Thank you for your report and for providing us with detailed information.

I was able to reproduce the issue on my end on Mac OSX 10.14.4 on:

  • Firefox Nightly 75.0a1 (2020-02-19) (64-bit).
  • Release 73.0.1 (64-bit)
  • Beta 74.0b5 (64-bit)

I'll add this ticket to the "Core: Graphics Layers" component and hopefully someone with more knowledge in this area will look over it.

Regards,
Virginia

Status: UNCONFIRMED → NEW
Component: Untriaged → Graphics: Layers
Ever confirmed: true
Product: Firefox → Core
Priority: -- → P3

layers.progressive-paint is an Android-only feature that has never been supported on desktop. There is no reason to flip this pref on desktop.

The release assertion added in 72 (in bug 1533918), which manifests as a crash if the pref is enabled on desktop, is related to sandboxing. The implementation of layers.progressive-paint uses a component (CrossProcessMutex) which can be used as a sandbox escape, and we want to make sure we don't accidentally instantiate this component on platforms with sandboxing enabled (which includes Linux).

Should probably be a P5; this is an unsupported configuration. The warning on about:config is there for precisely this reason :)

Priority: P3 → P5

Thanks for looking into the issue. I understand and agree with the reasoning and chosen solution in bug 1533918. However, is this actually documented anywhere? Even as an unsupported feature set, about:config is notoriously lacking in documentation (besides third-party projects such as MozillaZine, which currently does not cover this setting). I understand if this is out of scope for Firefox though.

Before potentially moving forward and closing this as unsupported configuration, I propose considering the following questions:

  • Should the flag be disabled in Safe Mode, akin to hardware acceleration? (I personally endorse this)
  • Is the crash-by-assertion appropriate and consistent behavior for an about:config entry? (likely yes)
  • Should information regarding the flag or its implications be documented somewhere? (depends on policy)

(In reply to Selis from comment #7)

Even as an unsupported feature set, about:config is notoriously lacking in documentation (besides third-party projects such as MozillaZine, which currently does not cover this setting). I understand if this is out of scope for Firefox though.

Documenting the prefs outside the actual codebase would impose an unreasonable overhead to developers adding or modifying prefs, which happens on a daily basis. The prefs are not intended to be modified by end users except in specific scenarios and so the cost/benefit of adding user-accessible documentation is just not worth it.

  • Should the flag be disabled in Safe Mode, akin to hardware acceleration? (I personally endorse this)

That's a not-unreasonable suggestion. I'd accept a patch that does that.

  • Is the crash-by-assertion appropriate and consistent behavior for an about:config entry? (likely yes)

Yes

  • Should information regarding the flag or its implications be documented somewhere? (depends on policy)

Not anywhere user-accessible. It's implicitly documented in the code and developers who need to flip it know can find out what it does by examining the code.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)

  • Should the flag be disabled in Safe Mode, akin to hardware acceleration? (I personally endorse this)

That's a not-unreasonable suggestion. I'd accept a patch that does that.

I would also accept a patch to ignore the layers.progressive-paint pref entirely on desktop.

I'll make this a mentored bug, happy to provide guidance to anyone who would like to write one of these patches.

Mentor: botond
Keywords: good-first-bug

I gave it a quick jab, just to try it. The patch is very rough, and not suitable for deployment in any way.

The patch is intended to prevent Progressive Paint from being used when Safe Mode is used, or when the platform is sandboxed.

Some notes:

  • I have no experience of working with the Firefox source code.
  • The patch may not actually do what is intended.
  • There may be changes needed that are not in the patch.
  • Some parts of the patch may be redundant, or otherwise never executed.
  • There are no tests.
  • The code has never been compiled, much less tested to work.

On top of that it probably fails in usual terms of code style, consistency etc.

Attachment #9129388 - Flags: feedback?(kats)
Comment on attachment 9129388 [details] Ignore Progressive Paint in Safe Mode and when Sandboxed Thanks for the patch! It's certainly in the right direction. If you want to pursue getting this patch landed, you can follow the steps at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions to get Firefox built and ensure that your patch compiles and has the intended effect (i.e. set the pref, then ensure it is ignored in safe mode). Once you have that I can do a review to address any style issues and we can get the patch landed.
Attachment #9129388 - Flags: feedback?(kats)

I’ll get trying on that.

Whiteboard: [apz-outreachy]
Mentor: kats
Whiteboard: [apz-outreachy] → [apz-outreachy][lang=c++]

If this is still available, may I work on it?

(In reply to fluffythefox from comment #13)

If this is still available, may I work on it?

Hi there! Yes, you're welcome to work on it.

If you haven't checked out and built the Firefox source code yet, that's a good starting point (see the link in comment 11) for instructions.

Once you've done that, you can apply the patch from comment 10 locally and get it working. Feel free to ask here if you have any questions, or find us in the #apz channel on Element.

(In reply to Botond Ballo [:botond] from comment #14)

(In reply to fluffythefox from comment #13)

If this is still available, may I work on it?

Hi there! Yes, you're welcome to work on it.

If you haven't checked out and built the Firefox source code yet, that's a good starting point (see the link in comment 11) for instructions.

Once you've done that, you can apply the patch from comment 10 locally and get it working. Feel free to ask here if you have any questions, or find us in the #apz channel on Element.

Will do. Cheers for the advice.

I'm having a bit of difficulty working out where to begin. I can't find a directory reflecting the attachment in this thread. Can you point me in the right direction?

(In reply to John from comment #16)

I'm having a bit of difficulty working out where to begin. I can't find a directory reflecting the attachment in this thread. Can you point me in the right direction?

Do you mean you're having trouble finding the files to which the attached patch applies?

I assume you've checked out the repository, which is at https://hg.mozilla.org/mozilla-central/, and built it as per the instructions linked above.

The files modified by the patch are gfx/layers/apz/src/AsyncPanZoomController.cpp and gfx/layers/client/ClientTiledPaintedLayer.cpp. They're more conveniently viewed in our Searchfox code search tool (e.g. AsyncPanZoomController.cpp).

You can apply the patch locally using hg import https://bug1614850.bmoattachments.org/attachment.cgi?id=9129388, though it's possible it doesn't apply cleanly due to the patch having bitrotted.

Let me know if that answers your question! You can also find us in https://chat.mozilla.org/#/room/#apz:mozilla.org to chat.

That was what I meant, my bad. I have built it according to the instructions, but I don't seem to have src under apz. I went through searchfox first before I asked for help. I'm looking under the mozilla-unified directory in src, correct me if that's the wrong dir.

(In reply to John from comment #18)

I don't seem to have src under apz

Ok, interesting.

What platform are you running on?

As a sanity check, could you run the following commands in the mozilla-unified directory, and paste their output?

  1. cat .hg/hgrc
  2. hg id
  3. ls
  4. ls gfx/layers/apz

Thanks!

(And also hg status for good measure.)

4 shows src in apz. My bad, seems tool I'm using just didn't note the directory. I was trying to use vscode instead of screwing around and setting up nvim for it. I think I'm good now.

Hi,
I am new to the open-source community and am interested in fixing this bug. Could I get some guidance, please?

Flags: needinfo?(kats)

(In reply to sudhanshuvshekhar from comment #22)

Hi,
I am new to the open-source community and am interested in fixing this bug. Could I get some guidance, please?

I was under the impression I was already working on this?

(In reply to sudhanshuvshekhar from comment #22)

Hi,
I am new to the open-source community and am interested in fixing this bug. Could I get some guidance, please?

Hi sudhanshuvshekhar,

The first step to working on any bug is going to be the same, which is to check out the code locally and get a working build of Firefox. You can follow the instructions at https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html to get that set up.

That being said, John has already expressed an interest in working on this particular bug and is actively working on it. To avoid duplication of effort, it would be better to pick another bug that sounds interesting to you and that doesn't have somebody actively working on it.

Hope that helps, and feel free to jump into the #introduction:mozilla.org or #apz:mozilla.org channel on https://chat.mozilla.org/ if you need more specific help.

Flags: needinfo?(kats)

It appears to be working. What's the process for submitting it for review? Any recommendations for testing other than replicating the procedure in the bug report?

Flags: needinfo?(botond)

https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html#to-submit-a-patch documents the steps to submit a patch (you need to install and use the moz-phab tool). I don't think it's worth adding an automated test for this bug since it's an unsupported scenario that users should normally not be running into. If the steps in comment 0 don't reproduce the crash with your patch that's probably good enough.

Flags: needinfo?(botond)

John, I'm assigning this bug to you to reflect that you're actively working on it.

Assignee: nobody → fluffythefox
Whiteboard: [apz-outreachy][lang=c++] → [lang=c++]

Hi John, any update here? Were you able to set up moz-phab to submit the patch?

Flags: needinfo?(fluffythefox)

(In reply to Kartikaya Gupta (email:kats@mozilla.staktrace.com) from comment #28)

Hi John, any update here? Were you able to set up moz-phab to submit the patch?

Sorry, some stuff came up along with exams. I forgot about this in the midst of it. I'll organise it Monday and submit it. Cheers for the reminder.

Flags: needinfo?(fluffythefox)

(In reply to Kartikaya Gupta (email:kats@mozilla.staktrace.com) from comment #28)

Hi John, any update here? Were you able to set up moz-phab to submit the patch?

Bootstrap fails. Can't get back up to date. Going to have to back out of this until I can get that working again. Not really sure where my work went, but can't set it up now. Is there somewhere I can report this specific issue? I have all the dependencies outlined in the documentation to set up for development.

Flags: needinfo?(kats)

If bootstrap is failing, please file a bug using this link and include the full output of the command. Thanks!

Flags: needinfo?(kats)

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Assignee: fluffythefox → nobody

Hi, I was able to reproduce this on 86.0, Linux Mint 20.1, so I'm guessing it's still outstanding. Is it alright if I work on it?

Sure, you're welcome to give it a try. Please read through the comments for how to get started, and let me know if you have any questions.

Assignee: nobody → 4gateftw
Status: NEW → ASSIGNED
Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/460f43279018 Fixed a crash when enabling layers.progressive_paint on desktop and sandboxed platforms. r=botond
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: