Firefox crashes on opening "about:support" with an WebExtension addon installed

VERIFIED FIXED in Firefox 45

Status

VERIFIED FIXED
3 years ago
4 months ago

People

(Reporter: rpl, Assigned: rpl)

Tracking

unspecified
mozilla45

Firefox Tracking Flags

(firefox45 fixed, firefox48 verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
Step to reproduce:

- install a WebExtension addon
- open "about:support" in a tab

Expected behaviour:

- new tab opened with "about:support" content rendered

Actual behaviour:

- Firefox crashes
(Assignee)

Comment 1

3 years ago
Created attachment 8678804 [details] [diff] [review]
0001-Bug-1218364-windowless-browser-windows-should-not-cr.patch

I'm attaching a patch with a testcase and a proposed fix.

The problem is obviously related to the windowless browser created by the WebExtension for the background page.

After a page is actually loaded in the windowless browser, its "layerManagerType" is NONE but the "ClientLayerManager::GetBackendName" method crash because it doesn't have a "NONE case branch" in the "switch...case" which resolve the type into a string name.

The above method ("ClientLayerManager::GetBackendName") is used by the "toolkit/modules/Troubleshoot.jsm" to calculate the number of the graphics  accelerate windows.

In the proposed fixes:

- add "LayersBackend::LAYERS_NONE" branch in the "ClientLayerManager::GetBackendName"'s switch-case
- change the Troubleshoot.jsm to recognize the "None" layerManagerType as non-accelerated

In the testcase:

- added browser chrome tests to the "gfx/test" dir 
  (apparently in the existent mochitest test suite I can't access the value sent by the "Troubleshoot.snapshot" method, it raise an exception because we can't access a privileged properties from the testcase context)
- create a new windowless browser and loads "about:blank" in it
- check the expected layerManagerType value
- check Troubleshoot.snapshot doesn't count the windowless browser as an accelerated window (maybe this part of the testcase should be moved elsewhere?)
Assignee: nobody → luca.greco
Status: NEW → ASSIGNED
Attachment #8678804 - Flags: review?(wmccloskey)

Updated

3 years ago
Duplicate of this bug: 1217879
Comment on attachment 8678804 [details] [diff] [review]
0001-Bug-1218364-windowless-browser-windows-should-not-cr.patch

Review of attachment 8678804 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for figuring this out Luca. I'm not sure this is the right approach though. Troubleshoot.jsm seems to take information about the compositor from the last window that it iterates over. If that happens to be one of these windowless browsers, then we'll get bad data.

I can think of two solutions. One is to make GetLayerManagerType throw for a windowless browser. Then Troubleshoot.jsm will skip it. The other is to have Troubleshoot.jsm skip non-browser windows. Perhaps it could use Services.wm.getEnumerator("navigator:browser").

Jeff, do you have any opinion here?
Attachment #8678804 - Flags: review?(wmccloskey) → review?(jmuizelaar)
(In reply to Bill McCloskey (:billm) from comment #3)
> 
> Jeff, do you have any opinion here?

I'd prefer if Troubleshoot.jsm skipped non-browser windows.
Comment on attachment 8678804 [details] [diff] [review]
0001-Bug-1218364-windowless-browser-windows-should-not-cr.patch

Review of attachment 8678804 [details] [diff] [review]:
-----------------------------------------------------------------

It seems like not iterating over windowsless browser windows is a better solution
Attachment #8678804 - Flags: review?(jmuizelaar) → review-
(Assignee)

Comment 6

3 years ago
Created attachment 8681724 [details] [diff] [review]
0001-Bug-1218364-windowless-browser-windows-should-not-cr.patch

Unfortunately enumerating "navigator:browser" doesn't skip the windowless browser.

How about skipping on windowLayerType == "None"'? can be a reasonable approach?

This updated patch apply the above solution (and add the related test steps in the test cas): 
- skip the windowless browser windows in the Troubleshoot.jsm graphics report by checking if windowLayerType == "None"
- Troubleshoot should not count a windowless browser in the numTotalWindow graphics report field, nor in the numAcceleratedWindow
- the presence of a windowless browser window should not set Troubleshoot's graphics.windowLayerManagerType to "None"
Attachment #8678804 - Attachment is obsolete: true
Attachment #8681724 - Flags: review?(wmccloskey)
Attachment #8681724 - Flags: review?(jmuizelaar)
Comment on attachment 8681724 [details] [diff] [review]
0001-Bug-1218364-windowless-browser-windows-should-not-cr.patch

Review of attachment 8681724 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me.
Attachment #8681724 - Flags: review?(jmuizelaar) → review+

Updated

3 years ago
Duplicate of this bug: 1219996
Attachment #8681724 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 9

3 years ago
I rebased this patch on a recent mozilla-central without have any conflicts or changes, follows the related try run (there are 10 intermittents which seem unrelated to this change):

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bd0ff92dbcd

"checkin-needed" keyword applied.
Keywords: checkin-needed
had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=5649889&repo=fx-team
Flags: needinfo?(luca.greco)
(Assignee)

Comment 13

3 years ago
Created attachment 8686056 [details] [diff] [review]
0001-Bug-1218364-windowless-browser-windows-should-not-cr.patch

My apologies, I didn't notice that the last "try run" was too much restricted for this kind of change, which definitely deserves a broader "try run" before the checkin-needed (and I learned an important lesson: "always put a platform different from the one you are working on when you launch a 'try run' restricted to a single platform")

From the above log we can assume that on OSX (and Windows) the existent windows (that are: the mochi test dialog and the firefox windows which runs the test cases) are always accellerated using OpenGL (and DirectX on Windows).

In the updated patch I applied the following changes:

- check layerManagerType != "None" (instead of assuming that it should be 'Basic')
- do not check the number of accelerated windows

Follows a couple of try runs:

- restricted to linux, osx and windows: https://treeherder.mozilla.org/#/jobs?repo=try&revision=36d8e7b29333
- all platforms: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5738f7a9eea
Attachment #8681724 - Attachment is obsolete: true
Flags: needinfo?(luca.greco)
Attachment #8686056 - Flags: review?(wmccloskey)
Attachment #8686056 - Flags: review?(jmuizelaar)
Attachment #8686056 - Flags: review?(jmuizelaar) → review+
Attachment #8686056 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 14

3 years ago
the above try runs confirms that this test case now passes on windows and osx consistently, I'm applying the checkin-needed keyword.
Keywords: checkin-needed
(Assignee)

Comment 17

3 years ago
The issue which has generated the backout doesn't seem to relate to this patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8167e895c1f
Keywords: checkin-needed

Comment 19

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7e66d2dc5c95
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

Updated

3 years ago
Iteration: --- → 45.2 - Nov 30
Duplicate of this bug: 1223184
I was able to reproduce the initial issue on Firefox 44.0a1 (2015-10-21) under Windows 10 64-bit.

Verified fixed on Firefox 48.0a1 (2016-03-24/25) under Windows 10 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.11.
Status: RESOLVED → VERIFIED
status-firefox48: --- → verified

Updated

4 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.