1284942 - Content script message listeners continue to fire for pages hidden in bfcache

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Alexis Rimbaud]

Attachment: test-1.0.xpi

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160614130443

Steps to reproduce:

I have attached a simple extension to test the bug. Simply navigate pages on a domain and see the console output when the page action icon is clicked.

The setup is as follows:

1. A runtime.onMessage listener is registered in a content script. The listener sends the content of the title tag as a response
2. When the page action icon is clicked, a message is sent from the background script to the corresponding tab


Actual results:

The bug occurs when browsing a domain in a single tab. When the page action is clicked, all the listeners registered on the previously opened pages are fired up and the response received by the caller is that of the first listener registered.


Expected results:

Only the listener registered by the current page should be run. Chrome does not exhibit the bug.

Note: navigating to a different domain in the same tab seems to exhibit a different bug (see the console output.)

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1284942: Don't fire message listeners for windows hidden in bfcache.

Review commit: https://reviewboard.mozilla.org/r/62756/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62756/

Comment 2

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/62756/#review59640

::: toolkit/components/extensions/ExtensionContent.jsm:477
(Diff revision 1)
>    }
>  
>    close() {
>      super.unload();
>  
> +    if (this.windowId === getInnerWindowID(this.contentWindow)) {

I'm confused by this, isn't the test basically the same as this.active?  Moreover, I don't understand why the listeners shouldn't be unconditionally removed?

::: toolkit/components/extensions/ExtensionUtils.jsm:1010
(Diff revision 1)
>        }).api(),
>        onMessage: new EventManager(this.context, "Port.onMessage", fire => {
>          let listener = ({data}) => {
> -          if (!this.disconnected) {
> +          if (!this.context.active) {
> +            // TODO: Send error as a response.
> +            Cu.reportError("Message received on port for an inactive content script");

Is this worthy of reportError?  It seems like it could be a common occurence.  In any case, why isn't it handled consistently between Port objects and runtime.onMessage below?

Comment 3

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/62756/#review59640

> I'm confused by this, isn't the test basically the same as this.active?  Moreover, I don't understand why the listeners shouldn't be unconditionally removed?

This may help: https://developer.mozilla.org/en-US/docs/Inner_and_outer_windows

The short answer is that the window object at this point may not point to the same inner window that we added the listeners to. And, for the most part it should be the same as `this.active`, yes, except that "pagehide" fires before this function gets called, so `this.active` will always be false at this point.

> Is this worthy of reportError?  It seems like it could be a common occurence.  In any case, why isn't it handled consistently between Port objects and runtime.onMessage below?

Ports are always connected to a specific content script, belonging to a specific inner window. `sendMessage` and `onMessage` are only targetted to a tab, and are meant to go to any active listeners in that tab. A message on a port to an inactive window is an error. An ordinary runtime to a tab with cached inner windows is just not meant to target them.

Comment 4

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/62756/#review59640

> This may help: https://developer.mozilla.org/en-US/docs/Inner_and_outer_windows
> 
> The short answer is that the window object at this point may not point to the same inner window that we added the listeners to. And, for the most part it should be the same as `this.active`, yes, except that "pagehide" fires before this function gets called, so `this.active` will always be false at this point.

Sure, but don't we have a separate instance of ExtensionContext per inner window that has a content script?  Even if the window object is now pointing to something else, we would just be removing the listener for the context object that corresponds to the inner window that is now being destroyed.  In the case where the inner window has changed, it seems like not removing the listeners here will leak the whole context since the listener will hold a reference to it that is never cleaned up?

> Ports are always connected to a specific content script, belonging to a specific inner window. `sendMessage` and `onMessage` are only targetted to a tab, and are meant to go to any active listeners in that tab. A message on a port to an inactive window is an error. An ordinary runtime to a tab with cached inner windows is just not meant to target them.

Ick, how does the sender know that a particular Port is in an inactive window?  I'm assuming onDisconnected doesn't happen until the window is actually destroyed.  Even if there was some notification to the sender, there would still be a race where the sender's message and the deactiviation notification could cross in flight.

Comment 5

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/62756/#review59640

> Sure, but don't we have a separate instance of ExtensionContext per inner window that has a content script?  Even if the window object is now pointing to something else, we would just be removing the listener for the context object that corresponds to the inner window that is now being destroyed.  In the case where the inner window has changed, it seems like not removing the listeners here will leak the whole context since the listener will hold a reference to it that is never cleaned up?

We do have separate contexts for each inner window, yes, but there's no way to access inactive inner windows from JavaScript.

In any case, we only get to this point when the inner window is being destroyed, so removing the listeners is not strictly necessary.

> Ick, how does the sender know that a particular Port is in an inactive window?  I'm assuming onDisconnected doesn't happen until the window is actually destroyed.  Even if there was some notification to the sender, there would still be a race where the sender's message and the deactiviation notification could cross in flight.

It doesn't.

Comment 6

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/62756/#review59640

> We do have separate contexts for each inner window, yes, but there's no way to access inactive inner windows from JavaScript.
> 
> In any case, we only get to this point when the inner window is being destroyed, so removing the listeners is not strictly necessary.

Sorry I think I'm missing your point.  The listeners are attached to the outer window but if an inactive inner window is destroyed, the corresponding listeners are not removed which I think leaks the associated context?  (at least until the outer window is destroyed and the whole mess becomes unreachable)

> It doesn't.

So back to the original point, this is something that can easily happen in practice and an extension author has no way to guard against it, so reportError() seems excessive.  In the long run, I think your TODO comment is right, generating a structured reply for the sender that they can reliably detect and handle seems like the best option.  reportError() seems like a reasonable placeholder until that is done, but can you file a bug for that if its not going to happen now?

Comment 7

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/62756/#review59640

> Sorry I think I'm missing your point.  The listeners are attached to the outer window but if an inactive inner window is destroyed, the corresponding listeners are not removed which I think leaks the associated context?  (at least until the outer window is destroyed and the whole mess becomes unreachable)

No, the listeners are attached to the inner window. The outer window is just a proxy.

> So back to the original point, this is something that can easily happen in practice and an extension author has no way to guard against it, so reportError() seems excessive.  In the long run, I think your TODO comment is right, generating a structured reply for the sender that they can reliably detect and handle seems like the best option.  reportError() seems like a reasonable placeholder until that is done, but can you file a bug for that if its not going to happen now?

Why would reportError seem excessive? It's something that they should know about, rather than something that we silently ignore.

There's already a bug to support replies to messages on ports. Or, at least, I think there is, but I can't find it.

Comment 8

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/62756/#review59640

> No, the listeners are attached to the inner window. The outer window is just a proxy.

Ah, well that's confusing.  In that case, why even bother manually removing the listeners?  It happens explicitly if the window is active and implicitly otherwise, why not just always do it one way (which has to be the implicit way)

> Why would reportError seem excessive? It's something that they should know about, rather than something that we silently ignore.
> 
> There's already a bug to support replies to messages on ports. Or, at least, I think there is, but I can't find it.

I don't think there's any real disagreement here, I said reportError() is a reasonable placeholder but it is suboptimal for lots of reasons (it doesn't actually notify the sender, it dumps something into the console where users are unlikely to notice it at the moment it happens for instance)

Comment 9

[Andrew Swan [:aswan]]

Comment on attachment 8768600 [details]
Bug 1284942: Don't fire message listeners for windows hidden in bfcache.

https://reviewboard.mozilla.org/r/62756/#review59782

Comment 10

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/62756/#review59640

> Ah, well that's confusing.  In that case, why even bother manually removing the listeners?  It happens explicitly if the window is active and implicitly otherwise, why not just always do it one way (which has to be the implicit way)

Yeah, outer windows are terrible. They're basically a side-effect of web APIs being designed in the mid-nineties before we had any idea how they'd be used.

As for why remove them at all, it's just the standard practice in Toolkit code, and the alternative is to use an ESLint rule override.

Comment 11

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/f87c66fa12c635e07cd071dbfb9646258b7896d2
Bug 1284942: Don't fire message listeners for windows hidden in bfcache. r=aswan

Comment 12

[Carsten Book [:Tomcat]]

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