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

RESOLVED FIXED in Firefox 50

Status

()

Toolkit
WebExtensions: Untriaged
P2
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Alexis Rimbaud, Assigned: kmag)

Tracking

47 Branch
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8768468 [details]
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.)
Assignee: nobody → kmaglione+bmo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P1
Summary: Listeners on runtime.onMessage in content scripts are persisted across pages of a domain → Content script message listeners continue to fire for pages hidden in bfcache
Created attachment 8768600 [details]
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/
Attachment #8768600 - Flags: review?(aswan)

Comment 2

2 years ago
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?
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

2 years ago
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.
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

2 years ago
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?
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

2 years ago
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

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

https://reviewboard.mozilla.org/r/62756/#review59782
Attachment #8768600 - Flags: review?(aswan) → review+
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.

Updated

a year ago
Priority: P1 → P2
Whiteboard: triaged
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f87c66fa12c6
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Duplicate of this bug: 1296609
You need to log in before you can comment on or make changes to this bug.