Closed Bug 1403349 Opened 2 years ago Closed 2 years ago

webNavigation.onCreatedNavigationTarget always fires with sourceTabId=-1 after browserAction popup closes via window.open('', '_self').close()

Categories

(WebExtensions :: Request Handling, defect, P1)

57 Branch
defect

Tracking

(firefox57 verified, firefox58 verified)

VERIFIED FIXED
mozilla58
Tracking Status
firefox57 --- verified
firefox58 --- verified

People

(Reporter: robwu, Assigned: rpl)

Details

Attachments

(7 files)

When the extension popup calls window.open('','_self').close(), then the next webNavigation.onCreatedNavigationTarget event will report sourceTabId -1.

STR:
1. Load attached extension at about:debugging
2. Open the global JS console.
3. Click on the extension button to show the extension popup, featuring two buttons inside.
4. Click on the first button (this will open a helper tab to avoid popup blocker).
5. Click on the first button again (this will now open a new tab via the helper tab).
6. Click on the second button (this will close the extension popup via:
   window.open('', '_self').close();
7. Look at the global JS console (Ctrl-Shift-J).

8. Click on the extension button to show the extension popup again.
9. Click on the first button again (like step 4, to open a new tab).
10. Look at the global JS console.

Expected:
After step 7 : onCreatedNavigationTarget, sourceTabId=2
After step 10: onCreatedNavigationTarget, sourceTabId=2
(sourceTabId can be any positive number, as long as it is not -1).

Actual:
After step 7 : onCreatedNavigationTarget, sourceTabId=2 (as expected)
After step 10: onCreatedNavigationTarget, sourceTabId=-1 (UNEXPECTED)


Extra information:
If you keep the extension popup open while executing step 10, and repeat step 6 (closing extension popup via second button), and then look at the global JS console, then you see:
onCreatedNavigationTarget, sourceTabId=-1

This event is generated by window.open('', '_self').close();

Because the bug seems to alternate between unexpected states, I think that this bug is caused by:
https://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/toolkit/modules/addons/WebNavigation.jsm#311-321

Reproduced in Firefox 55.0.3 and Firefox Nightly.
Originally reported by uBlock Origin; The discussion is on-going at: https://github.com/gorhill/uBlock/issues/3057
I immediately took a look at this issue (thanks Rob for the STR and the preliminary analysis),
follows some additional details about this issue:

- the changes introduced by Bug 1355120 (Get rid of top-level window ID tracking) has introduced the following change
  https://searchfox.org/mozilla-central/diff/307bb6e57e33361b3caf693f607ba82d42f1afe4/toolkit/modules/addons/WebNavigationContent.js#59:

```
-     const sourceWindowId = WebNavigationFrames.getDocShellWindowId(sourceDocShell);
-     const createdWindowId = WebNavigationFrames.getDocShellWindowId(createdDocShell);
+     const sourceFrameId = WebNavigationFrames.getDocShellFrameId(sourceDocShell);
+     const createdWindowId = WebNavigationFrames.getDocShellFrameId(createdDocShell);
```

  with this change `createdWindowId` is not the outerWindowId of the created docShell anymore, 
  but it is actually the frameId, which will be always 0 for a top level created frame.

  Because of this change, the code in `WebNavigation.jsm` is pairing two unrelated messages  
  and send completely wrong createdNavigationTarget events.

- when a window is opened using window.open but closed immediately, the event that we expect from the created window is
  never going to be received, and the map will never be cleared from that createdWindowId (it would not have created 
  exactly this bug without the changes applied by Bug 1355120, nevertheless it should have been removed from the map by 
  listening to the destroyed docshells and clearing them from the map if pending).

- besides going back to use the outerWindowId instead of the frameId (which will be basically 0 most of the times) for
  pairing the two onCreatedNavigationTarget messages, when there is more then a single content process 
  there is still a chance that the outerWindowId coming from two different processes can be the same, 
  to prevent further collitions the processID (related to the process where the outerWindowId has been created) 
  should be part of the key used for the Map of the pending CreatedNavigationTarget messages.
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Priority: -- → P1
(In reply to Luca Greco [:rpl] from comment #1)
> when there is more then a single content process 
>   there is still a chance that the outerWindowId coming from two different
> processes can be the same, 
>   to prevent further collitions the processID (related to the process where
> the outerWindowId has been created) 
>   should be part of the key used for the Map of the pending
> CreatedNavigationTarget messages.

outerWindowId is already unique across processes, there is no need to make the mapping complicated.

nsDOMWindowUtils::GetOuterWindowID() is defined here:
https://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/dom/base/nsDOMWindowUtils.cpp#2370

The value is delegated to:
https://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/dom/base/nsPIDOMWindow.h#527 WindowID()
https://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/dom/base/nsPIDOMWindow.h#726 mWindowId

mWindowId is initialized to NextWindowID():
https://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/dom/base/nsGlobalWindow.cpp#1035

NextWindowID() is implemented here, and is basically the bits of the process ID joined with a process-specific counter.
https://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/dom/ipc/ContentChild.cpp#3155
on Bug 1190687(In reply to Rob Wu [:robwu] from comment #2)
> outerWindowId is already unique across processes, there is no need to make
> the mapping complicated.

ah, that's true, and now I also remember that it has been mentioned also by billm when we were discussing about this part.
Attachment #8912850 - Flags: review?(aswan)
Comment on attachment 8912851 [details]
Bug 1403349 - Prevent WebNavigation jsm to leak the sourceTab browser on unpaired CreatedNavigationTarget messages.

https://reviewboard.mozilla.org/r/184190/#review189326

::: toolkit/modules/addons/WebNavigation.jsm
(Diff revision 1)
> -    if (isSourceTab) {
> -      sourceTabBrowser = browser;
> -      createdTabBrowser = pairedMessage.browser;
> -    } else {
> -      sourceTabBrowser = pairedMessage.browser;
> -      createdTabBrowser = browser;
> -    }

Hi Andrew,
I didn't added an r? assigned to you on this patch yet because I'm still taking a deeper look to be sure if the previous version of this code was actually right by assuming that the two events related to a "created navigation target" can be received in a different order:

I remember that there were obviously some differences between Firefox running in e10s mode or in a single process mode, but I did tried it multiple times and I've never received the message coming from the source tab before the event received by the created tab (nor the test cases are currently triggering this scenario, locally I've tried both manually and by executing the test cases multiple times in a row, and I also looked at the code coverage data collected by the building infrastructure).

In the meantime, I'm marking this as an issue, because we want to be absolutely sure that the order is always going to be the same before applying the change from this patch (and if turned out that the order of these two events may change, I'm going to find a different way to prevent a source/created browser to be leaked by this WebNavigation Map).
Comment on attachment 8912851 [details]
Bug 1403349 - Prevent WebNavigation jsm to leak the sourceTab browser on unpaired CreatedNavigationTarget messages.

https://reviewboard.mozilla.org/r/184190/#review189326

> Hi Andrew,
> I didn't added an r? assigned to you on this patch yet because I'm still taking a deeper look to be sure if the previous version of this code was actually right by assuming that the two events related to a "created navigation target" can be received in a different order:
> 
> I remember that there were obviously some differences between Firefox running in e10s mode or in a single process mode, but I did tried it multiple times and I've never received the message coming from the source tab before the event received by the created tab (nor the test cases are currently triggering this scenario, locally I've tried both manually and by executing the test cases multiple times in a row, and I also looked at the code coverage data collected by the building infrastructure).
> 
> In the meantime, I'm marking this as an issue, because we want to be absolutely sure that the order is always going to be the same before applying the change from this patch (and if turned out that the order of these two events may change, I'm going to find a different way to prevent a source/created browser to be leaked by this WebNavigation Map).

I've discussed a bit about this doubt about the ordering of these two messages with billm, to double-check with him that I was right and the order of these messages is going to be consistent (with the message from the created tab that is always going to be received before the one from the source tab, so that we can be sure that we can discard the source tab message if the message from the created tab has not been received yet):

The order of this messages is consistent because the observers subscribed by WebNavigationContent.js are going to be triggered in reverse order (See searchfox.org/mozilla-central/rev/f54c1723be/xpcom/ds/nsObserverList.cpp#76), and the observer subscribed by the created tab is going to be the last to be subscribed (and then the first one to be executed).

The only doubt was related to the behavior when window.open is used by specify a named window (and what was going to be the behavior, and order of these messages, when the named window doesn't exist yet and when it already exist.

(I added an additional patch to this mozreview queue, which adds these two scenarios to the "webNavigation on window.open" test cases).
Attachment #8912851 - Flags: review?(aswan)
Attachment #8913390 - Flags: review?(aswan)
Comment on attachment 8912850 [details]
Bug 1403349 - Fix wrong sourceTabId on webNavigation.onCreatedTarget event.

https://reviewboard.mozilla.org/r/184188/#review190796

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget_window_open.js:171
(Diff revision 1)
>    await BrowserTestUtils.removeTab(tab1);
>  
>    await extension.unload();
>  });
> +
> +add_task(async function test_on_created_navigation_target_browserAction_popup_window_open_close() {

nit: how about a shorter name but a comment describing the scenario being tested here

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget_window_open.js:213
(Diff revision 1)
> +
> +  clickBrowserAction(extension);
> +
> +  await extension.awaitMessage("browserAction_popup_executed");
> +
> +  info("open an url in a new tab from a window.open call");

nit: an -> a

::: toolkit/modules/addons/WebNavigation.jsm:316
(Diff revision 1)
> +      isSourceTab,
> +      sourceFrameId,
> +      url,
> +    } = data;
>  
> -    // We are going to potentially received two message manager messages for a single
> +    // We are going to received two message manager messages for a single

nit: received -> receive
overall though, thanks for clarifying this comment

::: toolkit/modules/addons/WebNavigationContent.js:19
(Diff revision 1)
> +  if (!docShell) {
> +    return undefined;
> +  }
> +
> +  return docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +                 .getInterface(Ci.nsIDOMWindow)

is this line needed?
Attachment #8912850 - Flags: review?(aswan) → review+
Comment on attachment 8912851 [details]
Bug 1403349 - Prevent WebNavigation jsm to leak the sourceTab browser on unpaired CreatedNavigationTarget messages.

https://reviewboard.mozilla.org/r/184190/#review190802

::: toolkit/modules/addons/WebNavigation.jsm:331
(Diff revision 3)
> +        // top level frame because the "webNavigation-createdNavigationTarget-from-js" observers
> +        // subscribed by WebNavigationContent.js are going to be executed in reverse order
> +        // (See http://searchfox.org/mozilla-central/rev/f54c1723be/xpcom/ds/nsObserverList.cpp#76)
> +        // and the observer subscribed to the created target will be the last one subscribed
> +        // to the ObserverService (and the first one to be triggered).
> +        throw new Error(

Why throw here?
Maybe `Cu.reportError()` would be more appropriate?

::: toolkit/modules/addons/WebNavigation.jsm:331
(Diff revision 3)
> +        // top level frame because the "webNavigation-createdNavigationTarget-from-js" observers
> +        // subscribed by WebNavigationContent.js are going to be executed in reverse order
> +        // (See http://searchfox.org/mozilla-central/rev/f54c1723be/xpcom/ds/nsObserverList.cpp#76)
> +        // and the observer subscribed to the created target will be the last one subscribed
> +        // to the ObserverService (and the first one to be triggered).
> +        throw new Error(

Why throw here?  This does seem like something that shouldn't happen but maybe just directly calling `Cu.reportError()` would be better?

::: toolkit/modules/addons/WebNavigation.jsm:337
(Diff revision 3)
> +      // Store a weak reference to the browser XUL element, so that we don't prevent
> +      // it to be garbage collected if it has been destroyed.
> +      const browserWeakRef = Cu.getWeakReference(browser);
> +
> +      this.createdNavigationTargetByOuterWindowId.set(createdOuterWindowId, {
> +        browserWeakRef,
> +        data,
> +      });
>        return;

nit: I think the overall flow here would be easier to follow if you pull this block out and put it before you even read `pairedMessage`.  Then you have all the logic for the first message first, followed by all the logic for the second message.

::: toolkit/modules/addons/WebNavigation.jsm:354
(Diff revision 3)
> -      sourceTabBrowser = browser;
> -      createdTabBrowser = pairedMessage.browser;
> -    } else {
> -      sourceTabBrowser = pairedMessage.browser;
> +      throw new Error(
> +        `Discarding onCreatedNavigationTarget for ${createdOuterWindowId}: ` +
> +        "the created tab has been already destroyed"
> +      );

Why throw here?  It may be uncommon, but this is something that can happen in a regular (non-error) situation.
Attachment #8912851 - Flags: review?(aswan) → review+
Comment on attachment 8913390 [details]
Bug 1403349 - Add a new webNavigation test for window.open called with a named window target.

https://reviewboard.mozilla.org/r/184728/#review190808

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget_window_open.js:241
(Diff revision 1)
>    await BrowserTestUtils.removeTab(tab1);
>  
>    await extension.unload();
>  });
> +
> +add_task(async function test_on_created_navigation_target_browserAction_window_open_in_named_win() {

This might be easier to describe in a comment than in a function name.

::: browser/components/extensions/test/browser/browser_ext_webNavigation_onCreatedNavigationTarget_window_open.js:257
(Diff revision 1)
> +
> +  await extension.startup();
> +
> +  const expectedSourceTab = await extension.awaitMessage("expectedSourceTab");
> +
> +  info("open an url in a new named window from a window.open call");

nit: an -> a (also below)
Attachment #8913390 - Flags: review?(aswan) → review+
Comment on attachment 8912850 [details]
Bug 1403349 - Fix wrong sourceTabId on webNavigation.onCreatedTarget event.

https://reviewboard.mozilla.org/r/184188/#review190796

> nit: how about a shorter name but a comment describing the scenario being tested here

I changed it into `test_window_open_close_from_browserAction_popup`, which is way shorter but still easily recognizable in the test logs (which is my main reason for keeping the test functions names descriptive enough).

(I've not added a comment because I think that the new function names still makes clear which is the scenario, the removed part of the function name wasn't really adding anything useful, given that all the tests in this test file are related to the onCreatedNavigationTarget event)

> is this line needed?

Yeah, it is needed (I guess that there could be also other paths to reach the DOMWindowUtils from the docShell instance, but this seems to be the commonly used one).
Comment on attachment 8912851 [details]
Bug 1403349 - Prevent WebNavigation jsm to leak the sourceTab browser on unpaired CreatedNavigationTarget messages.

https://reviewboard.mozilla.org/r/184190/#review190802

> Why throw here?  This does seem like something that shouldn't happen but maybe just directly calling `Cu.reportError()` would be better?

I changed it into a `Services.console.logStringMessage` 

(the main reason why I'm not very keen to discard this data silently is that this scenario should not be very common and by logging a message we could more easily diagnose further regressions related to it, but is still a possible scenario and "marking it as an error" could be misleading).

> Why throw here?  It may be uncommon, but this is something that can happen in a regular (non-error) situation.

Changed into Services.console.logStringMessage as for the other one (for the same reason described above).
Comment on attachment 8913390 [details]
Bug 1403349 - Add a new webNavigation test for window.open called with a named window target.

https://reviewboard.mozilla.org/r/184728/#review190808

> This might be easier to describe in a comment than in a function name.

changed to `test_window_open_in_named_win`.
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/0039b4539113
Fix wrong sourceTabId on webNavigation.onCreatedTarget event. r=aswan
https://hg.mozilla.org/integration/autoland/rev/5dbe3db9affc
Prevent WebNavigation jsm to leak the sourceTab browser on unpaired CreatedNavigationTarget messages. r=aswan
https://hg.mozilla.org/integration/autoland/rev/6c50dc08aa42
Add a new webNavigation test for window.open called with a named window target. r=aswan
Backed out in https://hg.mozilla.org/integration/autoland/rev/f290517125dc for https://treeherder.mozilla.org/logviewer.html#?job_id=136624626&repo=autoland and friends. Apparently browser_ext_webNavigation_onCreatedNavigationTarget_window_open.js was already on the edge of taking too long to run, thus bug 1353676, but adding in your test took it over the edge, permaorange on the slowest platforms and intermittent on faster.
Attachment #8919313 - Flags: review?(aswan)
Comment on attachment 8919313 [details]
Bug 1403349 - Split webNavigation onCreatedNavigationTarget tests to prevent timeouts on linux32 debug.

https://reviewboard.mozilla.org/r/190180/#review195924

You may have already looked into this but at least for xpcshell, we can add custom `head = ...` directives to individual test cases.  Does that work for browser chrome tests?  If it does, that would be a nice alternative to all the `loadSubscript()` calls.
Attachment #8919313 - Flags: review?(aswan) → review+
Comment on attachment 8919313 [details]
Bug 1403349 - Split webNavigation onCreatedNavigationTarget tests to prevent timeouts on linux32 debug.

https://reviewboard.mozilla.org/r/190180/#review195924

I definitely agree with you, it would be nicer if we could use the "head = ..." in the browser.ini file instead of the loadSubscript() workaround, I've tried but without any luck and it looks that none of the other browser.ini file are using the head directive in a test file section and so my guess is that is not currently supported.

I'm going to land this like it is in the meantime (after all, the other tests in this directory use the same strategy and so at least it is consistent), but I can do it in a follow up if I'm wrong and it is actually supported (but very well hidden :-))
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/435db72f9b6a
Fix wrong sourceTabId on webNavigation.onCreatedTarget event. r=aswan
https://hg.mozilla.org/integration/autoland/rev/6692df0c5687
Prevent WebNavigation jsm to leak the sourceTab browser on unpaired CreatedNavigationTarget messages. r=aswan
https://hg.mozilla.org/integration/autoland/rev/acc39e04d4f0
Add a new webNavigation test for window.open called with a named window target. r=aswan
https://hg.mozilla.org/integration/autoland/rev/e802283bbf21
Split webNavigation onCreatedNavigationTarget tests to prevent timeouts on linux32 debug. r=aswan
Comment on attachment 8912850 [details]
Bug 1403349 - Fix wrong sourceTabId on webNavigation.onCreatedTarget event.

Approval Request Comment

[Feature/Bug causing the regression]:
Bug 1355120 (get rid of top-level window ID tracking) has introduced this
regression in Firefox 55.

[User impact if declined]:
As described in Comment 0 and in the related uBlock issue report (https://github.com/gorhill/uBlock/issues/3057), the uBlock Popup blocking becomes broken after opening uBO's dashboard through popup panel.

[Is this code covered by automated tests?]:
Yes

[Has the fix been verified in Nightly?]:
No (not yet)

[Needs manual test from QE? If yes, steps to reproduce]: 
Yes, it would be great to also verify it on uBlock using the
STR described in the upstream issue (https://github.com/gorhill/uBlock/issues/3057#issue-260745969):

1. Launch Firefox (and install uBlock origin if not yet installed)
2. Open popup test page in a new tab.(http://raymondhill.net/ublock/popup.html)
3. Verify that the popup from the first test ("direct url tab") is properly blocked
4. Open uBO's dashboard (or logger) through uBO's browserAction popup panel
5. Go back to the popup test page previously opened above, and repeat the test in 3.

[List of other uplifts needed for the feature/fix]:
None (besides the set of patches attached to this issue)

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
The changes are restricted to the WebExtensions webNavigation API internals, none of the other Firefox internals or WebExtensions APIs should be affected by this change
(and all other changes are test-only).

Also, the only WE webNavigation API internals that are changed by these patches are the ones related to the regressed feature.

[String changes made/needed]:
None
Attachment #8912850 - Flags: approval-mozilla-beta?
I would like to take this one if we can. It impacts uBlock Origin in some cases https://github.com/gorhill/uBlock/issues/3057. I think its a pretty unusual edge case, so the impact is limited to those edge cases, but uBlock Origin is one of the biggest add-ons we have.
Attached file Desktop.zip
I can reproduce this issue on Firefox 57.0b10 (20171019140425) under Wind 7 64-bit. 

This issue is verified as fixed on Firefox 58.0a1 (20171023100252) under Wind 7 64-bit and Mac OS X 10.13.

Using the STR from comment 32 and comment 0, I can see that in the browser console the next message: “Discarding onCreatedNavigationTarget for 4294967362: received source tab data without any created tab data available” is displayed.

Please see the attached screenshots.
Status: RESOLVED → VERIFIED
Comment on attachment 8912850 [details]
Bug 1403349 - Fix wrong sourceTabId on webNavigation.onCreatedTarget event.

Must fix as it affects a top webext, beta57+
Attachment #8912850 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attached image FixedBeta.png
This issues is verified as fixed on Firefox 57.0b11 (20171023180840) under Wind 7 64-bit and Mac OS X 10.13

Please see the attached screenshot.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.