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

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

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

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 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?)

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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?

Comment 4

[Jeff Muizelaar [:jrmuizel]]

(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 5

[Jeff Muizelaar [:jrmuizel]]

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

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 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"

Comment 7

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

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.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/55b6a42400b0

Comment 11

[Carsten Book [:Tomcat]]

had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=5649889&repo=fx-team

Comment 12

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/512d39a8a810

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 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

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

the above try runs confirms that this test case now passes on windows and osx consistently, I'm applying the checkin-needed keyword.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e23d0660a042

Comment 16

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/2164981a754e

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

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

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

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7e66d2dc5c95

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7e66d2dc5c95

Comment 21

[Vasilica Tamas, QA [:vtamas]]

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.