1301837 - SwapDocShells event fires twice for one swap

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Rob Wu [:robwu]]

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

Comment 1

[Rob Wu [:robwu]]

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?

Comment 2

[Drew Willcoxon :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?

Comment 3

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

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.

Comment 4

[Rob Wu [:robwu]]

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

Comment 5

[Rob Wu [:robwu]]

Attachment: Bug 1301837 - Defer SwapDocShells event registration in MessageManagerProxy

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9dff7536af4
Defer SwapDocShells event registration in MessageManagerProxy r=rpl

Comment 7

[Arthur Iakab [arthur_iakab]]

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

Comment 8

[Madalin Cotetiu]

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