Closed Bug 1293583 Opened 5 years ago Closed 4 years ago

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

Categories

(WebExtensions :: Untriaged, defect)

51 Branch
defect
Not set
normal

Tracking

(firefox51 affected)

RESOLVED WORKSFORME
Tracking Status
firefox51 --- affected
Blocking Flags:
webextensions ?

People

(Reporter: robwu, Assigned: robwu)

Details

(Whiteboard: triaged)

Attachments

(1 file)

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
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 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
Attachment #8779219 - Flags: review?(wmccloskey) → review+
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.
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f90cbdbd61d2
add test that checks whether the main frame has frame ID 0 r=billm
Keywords: checkin-needed
Whiteboard: triaged
Rob, since this test passes, I'm not clear how it helps with the particular problem this bug describes.
Flags: needinfo?(rob)
webextensions: --- → ?
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.
Flags: needinfo?(rob)
The test passes when run by itself.  Since there have been no further reports attached here, I'm closing.
Assignee: nobody → rob
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WORKSFORME
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.