Closed Bug 1301837 Opened 3 years ago Closed 7 months ago

SwapDocShells event fires twice for one swap

Categories

(WebExtensions :: General, defect)

51 Branch
defect
Not set

Tracking

(firefox51 wontfix, firefox68 fixed)

RESOLVED FIXED
mozilla68
Tracking Status
firefox51 --- wontfix
firefox68 --- fixed

People

(Reporter: robwu, Assigned: robwu)

References

Details

Attachments

(1 file)

This is the implementation of browser.swapDocShells(anotherBrowser) [1]:

  let event = new CustomEvent("SwapDocShells", {"detail": aOtherBrowser});
  this.dispatchEvent(event);
  event = new CustomEvent("SwapDocShells", {"detail": this});
  aOtherBrowser.dispatchEvent(event);

If I want to track the message manager for a <browser>, naturally I would do something along the lines of:

  function followBrowser(browser, setMessageManager) {
    browser.addEventListener("SwapDocShells", function listener({detail: newBrowser}) {
      browser.removeEventListener("SwapDocShells", listener);
      followBrowser(newBrowser, setMessageManager);
    });
    setMessageManager(browser.messageManager);
  }
  followBrowser(browser, (mm) => { /* do something with mm */ });

However, there is a problem. Given two browsers A and B, the behavior differs depending on whether A.swapDocShells(B) or B.swapDocShells(A) is called:

  let fooMM;
  followBrowser(A, (mm) => fooMM = mm);
  // Now fooMM = A.messageManager
  A.swapDocShells(B);
  // Expected: fooMM == B.messageManager
  // Actual: fooMM == A.messageManager
  // because the first SwapDocShells event causes a listener to be added to B.
  // and then the listener is fired on B, which triggers the new listener and undoes the changes.

If I use B.swapDocShells(A) instead, fooMM is B.messageManager as expected.


There are multiple ways to solve this, e.g.:
- Somehow get all current listeners before dispatching the event.
- Add an extra parameter to event.detail to allow the listener to identify the event (e.g. an integer, then the listener can store it and check whether it is equal).
- Require browser.swapDocShells to be called only on new <browser>s.
- Remove one of the dispatchEvent calls (it was added in bug 1019990).

[1] http://searchfox.org/mozilla-central/rev/124c120ce9d7e8407c9ad330a9d6b1c4ad432637/toolkit/content/widgets/browser.xml#1207
To see the above in action:

Open two tabs, example.com and example.net (both in the same process).

followBrowser(gBrowser.browsers[0], mm => console.log('foo'));
// Logs "foo MM"
gBrowser.browsers[0].swapDocShells(gBrowser.browsers[1]);
// Logs "foo MM" twice.

followBrowser(gBrowser.browsers[0], mm => console.log('bar'));
// Logs "bar MM"
gBrowser.browsers[1].swapDocShells(gBrowser.browsers[0]);
// Logs "bar MM" once.


Drew (patch author), Tim (reviewer): What should be done here?
Flags: needinfo?(ttaubert)
Flags: needinfo?(adw)
Hi Rob, sorry for the huge delay.  I don't think that this is necessarily the responsibility of the code at [1] to handle, but if the annoyance were to keep coming up in different contexts, I might change my mind.  I think what you can do, and what I would probably do, is defer the recursion a turn of the event loop, i.e., after both SwapDocShells events have been fired:

function followBrowser(browser, setMessageManager) {
  browser.addEventListener("SwapDocShells", function listener({detail: newBrowser}) {
    browser.removeEventListener("SwapDocShells", listener);
    setTimeout(() => {
      followBrowser(newBrowser, setMessageManager);
    });
  });
  setMessageManager(browser.messageManager);
}

Or you could ignore the second event:

let first = true;
function followBrowser(browser, setMessageManager) {
  browser.addEventListener("SwapDocShells", function listener({detail: newBrowser}) {
    if (first) {
      first = false;
      browser.removeEventListener("SwapDocShells", listener);
      followBrowser(newBrowser, setMessageManager);
    } else {
      first = true;
    }
  });
  setMessageManager(browser.messageManager);
}

I haven't tested these but I think both should work?
Flags: needinfo?(ttaubert)
Flags: needinfo?(adw)
In practice I think this isn't so much of a problem for individual browsers. We always call swapDocShells in this order newBrowser.swapDocShells(oldBrowser) and then we close oldBrowser's tab shortly after that. newBrowser is a browser for a new tab that was just added. Presumably you'll only have listeners on oldBrowser. The event is fired first on newBrowser and then on oldBrowser. So if your oldBrowser listener adds a SwapDocShells listener on newBrowser, then that listener won't fire (until another swap happens).

I realize this isn't great. It breaks down if you install a listener on the entire window, for example.

Bug 1279086 introduced the "EndSwapDocShells" event:
https://hg.mozilla.org/mozilla-central/rev/cfcd8c4f3a36b958002010a9c6d461a7769996d5#l36.2

That can be used to avoid the double-docswapping issue, by deferring the registration of the SwapDocShells event until the EndSwapDocShells event, similarly to how bug 1515077 was fixed.

That needs to be done here too:
https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/toolkit/components/extensions/MessageManagerProxy.jsm#167,192,195

Assignee: nobody → rob
Status: NEW → ASSIGNED
Depends on: 1279086
Product: Toolkit → WebExtensions
See Also: → 1515077

The "SwapDocShells" event should be deferred until "EndSwapDocShells".
Otherwise the event MessageManagerProxy may swap the event listeners
twice, and end up having the listeners on the incorrect message manager.

Depends on D27288

See Also: → 1545439
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/f9dff7536af4
Defer SwapDocShells event registration in MessageManagerProxy r=rpl
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Can you please provide some STR for this issue so we can check it manually? If no manual testing is needed please mark it as "qe-verify- "

Flags: needinfo?(rob)
Flags: needinfo?(rob) → qe-verify-
You need to log in before you can comment on or make changes to this bug.