[e10s] Add-on multiprocess compatible shims stops TabSelect event listeners being removed from the tab container

RESOLVED FIXED in Firefox 47

Status

()

defect
P1
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: standard8, Assigned: gkrizsanits)

Tracking

({addon-compat})

Trunk
Firefox 48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox45 wontfix, firefox46+ wontfix, firefox47+ fixed, firefox48+ fixed)

Details

Attachments

(3 attachments)

In Hello, we're listening to TabSelect events when we're doing browser tab sharing. We noticed bug 1264334, which caused more investigation.

When we start browser sharing we essentially do:

window.gBrowser.tabContainer.addEventListener("TabSelect", this);

When we stop it we do:

window.gBrowser.tabContainer.removeEventListener("TabSelect", this);

However, we're seeing in both non-e10s and e10s modes, that the TabSelect event still fires after it has been removed.

If we declare Hello as e10s compatible, via install.rdf (which we can't do at the moment due to bug 1262560):

<em:multiprocessCompatible>true</em:multiprocessCompatible>

then the TabSelect listeners get removed correctly, and everything works as we expect.

I'm therefore assuming this issue is in the shim code, since that's the only change I'm making here.

This is also potentially bad for existing add-ons as it happens in non-e10s mode.

I've reproduced on FF 47 & 48. I've not tried 45 or 46 since we're not really compatible there.
[Tracking Requested - why for this release]: This feels like it could adversely affect non-e10s compatible add-ons.
This is also broken with:

  gBrowser.addEventListener("mousemove", this);
  gBrowser.addEventListener("click", this);

and

  gBrowser.removeEventListener("mousemove", this);
  gBrowser.removeEventListener("click", this);
Here's a reduced test add-on that's not marked as e10s compatible.

STR

1) Start up Firefox
2) In about:config set xpinstall.signatures.required to false.
3) Install the add-on (don't restart, as for some reason it doesn't re-install the menu item).
4) Ensure more than one tab open
5) Open the browser console so it can be seen whilst using the browser window.
6) Select Tools -> Toggle Listeners

=> Adding listeners is displayed on the console

7) Move mouse around the browser area, click and also select different tabs

=> The names of the events get displayed on the console

8) Select Tools -> Toggle Listener again

=> Removing Listeners is displayed

7) Move mouse around the browser area, click and also select different tabs

Expected Results

=> No event names get printed

Actual Results

=> The names of the mousemove and TabSelect events get displayed on the console

(for some reason "click" doesn't get displayed).
Here's a reduced test add-on that is marked as e10s compatible. The only difference is the version number and the following addition in install.rdf:

    <em:multiprocessCompatible>true</em:multiprocessCompatible>

STR

1) Start up Firefox
2) In about:config set xpinstall.signatures.required to false.
3) Install the add-on (don't restart, as for some reason it doesn't re-install the menu item).
4) Ensure more than one tab open
5) Open the browser console so it can be seen whilst using the browser window.
6) Select Tools -> Toggle Listeners

=> Adding listeners is displayed on the console

7) Move mouse around the browser area, click and also select different tabs

=> The names of the events get displayed on the console

8) Select Tools -> Toggle Listener again

=> Removing Listeners is displayed

7) Move mouse around the browser area, click and also select different tabs

Expected & Actual results match:

=> No event names get printed on the console
[Tracking Requested - why for this release]: With the add-ons, I've verified this is an issue in all active trains from 45.0.2 to latest nightly.

The effect on users will vary depending on which add-ons they have installed, and if the add-ons are frequently adding/removing listeners - maybe not for most add-ons.

In Hello, Ian's been seeing strange results when we're doing tab sharing multiple times without restarting the browser, and I think the fact we're stacking listeners up is a likely cause.

I don't have enough knowledge to know the full potential impact on users - it may be small - but raising it to drivers for further investigation.
Jorge is this something you can help with? Gabor I think you worked on e10s addon shims, maybe you have some insight or know who to pass this along to.
Flags: needinfo?(jorge)
Flags: needinfo?(gkrizsanits)
Mike, I don't want to bother Bill with reviews... could you please take a look at this one?
Assignee: nobody → gkrizsanits
Flags: needinfo?(gkrizsanits)
Attachment #8741354 - Flags: review?(mconley)
Thanks Gabor, I've just run this through Hello's cases and it seems to work fine!

(obviously letting Mike review it though).
This seems innocent enough, but I don't actually understand how it's fixing things. Can you explain where the problem is?
Flags: needinfo?(gkrizsanits)
Flags: needinfo?(jorge)
Keywords: addon-compat
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #9)
> This seems innocent enough, but I don't actually understand how it's fixing
> things. Can you explain where the problem is?

The shim layer is using a filtering listener instead of the original one for _most_ of the events:
target.addEventListener(type, makeFilteringListener(type, listener), useCapture, wantsUntrusted);
same for removal:
target.removeEventListener(type, makeFilteringListener(type, listener), useCapture);

But not for ["mousedown", "mouseup", "click"];

Pre-patch, when we add a click event listener if the same listener for some other event was already added, then we return that filteringListener from makeFilteringListener (the filtering listener weakmap does not care about the event type). That's not what we want. So for click what's happening at add and remove is based on if the listener is already registered for some other event or not. This totally confuses everything.
Flags: needinfo?(gkrizsanits)
Comment on attachment 8741354 [details] [diff] [review]
makeFilteringListener breaks event removal with shims. v1

Review of attachment 8741354 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, okay, I think I understand. Thanks!

I'd love to see some regression tests for this, btw. Follow-up fodder if needs be.
Attachment #8741354 - Flags: review?(mconley) → review+
Blocks: e10s-addons
Priority: -- → P1
Is this still something you are aiming to uplift to beta 46? I am building beta 11 today. 
This could still make the RC build if it is worth the risk.
Flags: needinfo?(standard8)
Flags: needinfo?(gkrizsanits)
From jorge earlier today: Potentially affects hundreds of add-ons, since TabSelect is a very commonly-used event. As I understand it, the problem is that the event listener isn't being removed correctly, which is a relatively minor problem (a little bit of memory being used unnecessarily, and possibly some JS errors in the console). It's unlikely to break add-ons.
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #13)
> From jorge earlier today: Potentially affects hundreds of add-ons, since
> TabSelect is a very commonly-used event. As I understand it, the problem is
> that the event listener isn't being removed correctly, which is a relatively
> minor problem (a little bit of memory being used unnecessarily, and possibly
> some JS errors in the console). It's unlikely to break add-ons.

Yeah having thought about it a bit more, I think its not too bad. For Hello, it could be causing more issues if people have multiple Hello sessions within one browser session, but its hard for us to quantify that effect at the moment.

I'm thinking that we can probably just ship in 47, since the issue is in 45 already and we haven't heard major complaints about it from the Hello perspective.
Flags: needinfo?(standard8)
Let's aim this at 47 then. Thanks Mark, Gabor, and Jorge!
Flags: needinfo?(gkrizsanits)
(In reply to Mark Banner (:standard8) from comment #8)
> Thanks Gabor, I've just run this through Hello's cases and it seems to work
> fine!

Thanks for the polished bug report it really helped.

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #15)
> Let's aim this at 47 then. Thanks Mark, Gabor, and Jorge!

Alright, it looks green on try, I'll flag it for aurora only then.

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #11)
> I'd love to see some regression tests for this, btw. Follow-up fodder if
> needs be.

I'm not sure when will I have the bandwidth for it, I'll try to get back to it soon.
https://hg.mozilla.org/mozilla-central/rev/2e1fb077ecdb
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment on attachment 8741354 [details] [diff] [review]
makeFilteringListener breaks event removal with shims. v1

Approval Request Comment
[Feature/regressing bug #]: Bug 1059207
[User impact if declined]: "mousedown", "mouseup", "click" event listeners from addons when shims are turned on (which is now the default unless the add-on sets the compatible flag in the manifest) can confuse things. The side effect depends on the other listeners. Normally we would not use shims for these messages, but without this patch we might end up using them. Plus in the event removal it can cause other troubles like in this bug, causing some other event listeners refusing their removal requests...
[Describe test coverage new/current, TreeHerder]: I don't have any automated test for this. The patch is landed on mc.
[Risks and why]: This patch is quite trivial.
[String/UUID change made/needed]: none
Attachment #8741354 - Flags: approval-mozilla-aurora?
Comment on attachment 8741354 [details] [diff] [review]
makeFilteringListener breaks event removal with shims. v1

Improves Hello + e10s, Aurora47+
Attachment #8741354 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.