1293583 - non-zero main frame ID from ExtensionManagement.getFrameId (used by webRequest and webNavigation)

Status: RESOLVED WORKSFORME

(WebExtensions :: Untriaged, defect)

Description

[Rob Wu [:robwu]]

If a tab is opened before ExtensionManagement.jsm is loaded, then the frame ID of the main frame as reported by webNavigation.getAllFrames is NOT zero. This breaks APIs such as tabs.executeScript, which expects that the frameId of the top frame is 0 [1]. It can also cause intermittent test failures.

This incorrect ID is caused by the fact that the set of top-level IDs is maintained by subscribing to Extension:TopWindowID from the content processes [2]. If the module is not loaded in the main process when an ExtensionGlobal is created for the page, then the main process does not know that the outer window ID should be converted to 0.

I discovered this problem when I ran the browser_ext_tabs_executeScript.js test in isolation; the test failed because the frameId of the main frame was reported as non-zero.


[1] http://searchfox.org/mozilla-central/rev/5b35b60ada19621c84ecab78998416d8b86a8254/toolkit/modules/addons/WebNavigationFrames.jsm#89
[2] http://searchfox.org/mozilla-central/rev/5b35b60ada19621c84ecab78998416d8b86a8254/toolkit/components/extensions/ExtensionManagement.jsm#47

Comment 1

[Rob Wu [:robwu]]

Attachment: Bug 1293583 - add test that checks whether the main frame has frame ID 0

Tests cannot be run in a new process, so when this test is run along
with other tests, the faulty behavior is not caught.
You can manually check whether the bug is still present by running the
test in isolation:

    mach mochitest browser/components/extensions/test/browser/browser_ext_webNavigation_frameId0.js

Review commit: https://reviewboard.mozilla.org/r/70264/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70264/

Comment 2

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

Huh. ExtensionManagement used to be loaded by nsBrowserGlue. I guess that was removed at some point, which introduced this bug. We should probably just put it back there.

Comment 3

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

Comment on attachment 8779219 [details]
Bug 1293583 - add test that checks whether the main frame has frame ID 0

https://reviewboard.mozilla.org/r/70264/#review68532

Comment 4

[Rob Wu [:robwu]]

Requesting to land the test without fixing the bug.
The test passes anyway due to the fact that all tests are run in the same process (whereas the bug only shows up if the test is run in isolation), so it's good to land.

The test is useful for those who want to fix or debug the issue.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f90cbdbd61d2
add test that checks whether the main frame has frame ID 0 r=billm

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f90cbdbd61d2

Comment 7

[Shane Caraveo (:mixedpuppy)]

Rob, since this test passes, I'm not clear how it helps with the particular problem this bug describes.

Comment 8

[Rob Wu [:robwu]]

The test fails when it is the only test that is being run (at least, that was the case when I reported this bug - I did not check whether it is still the case). So if you develop locally and run the added test only, then you can see whether your patch will fix the bug for real.

See comment 4 and the comments in the test file for more details.

Comment 9

[Shane Caraveo (:mixedpuppy)]

The test passes when run by itself.  Since there have been no further reports attached here, I'm closing.

Comment 10

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.