Capturing load/DOMContentLoaded/DOMWindowCreated/DocumentElementInserted listeners need to check their targets

RESOLVED FIXED in Firefox 44, Firefox OS v2.5

Status

()

Toolkit
WebExtensions: Untriaged
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kmag, Assigned: kmag, Mentored)

Tracking

unspecified
mozilla45
Points:
---
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox44 fixed, firefox45 fixed, b2g-v2.5 fixed)

Details

(Whiteboard: [good first bug])

Attachments

(2 attachments)

This only happens when using `"backgound": {"scripts": ...` in the manifest. It presumably also happens for any other element which emits load events (images).

If the element in question is injected by one of those background scripts, it results in an infinite loop.

Comment 1

2 years ago
Hey i would like to work on this bug !! Can u please help me how to go about it
(In reply to Shivam from comment #1)
> Hey i would like to work on this bug !! Can u please help me how to go about
> it

Certainly.

You should start by checking out the source code for Firefox, and making sure you can build it, if you haven't done it before: https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

The code that loads the background scripts is here: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ext-backgroundPage.js

If you have any questions, feel free to ask here or in irc://irc.mozilla.org/webextensions

Updated

2 years ago
Flags: blocking-webextensions+
I'm just going to retarget this bug, since we do this in several places.
Summary: Adding an iframe to a background page causes background scripts to be re-loaded → Capturing load/DOMContentLoaded/DOMWindowCreated/DocumentElementInserted listeners need to check their targets
Created attachment 8687636 [details] [diff] [review]
[webext] Check capturing event listeners for the correct target

This fixes the issues with events being processed for the wrong
targets. I'm worried that some of these event handlers are still a
bit shaky, though. The "unload" handler in Extension.jsm, for
instance, definitely does not fire for pages that end up in bfcache.
And I'm pretty sure we still handle some events even if the docshell
navigates to a new page before they've fired.
Attachment #8687636 - Flags: review?(wmccloskey)
Assignee: nobody → kmaglione+bmo
Status: NEW → ASSIGNED
Comment on attachment 8687636 [details] [diff] [review]
[webext] Check capturing event listeners for the correct target

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

::: toolkit/components/extensions/Extension.jsm
@@ +321,5 @@
>      inject(extension, context);
>  
>      let eventHandler = docShell.chromeEventHandler;
>      let listener = event => {
> +      if ((event.target.defaultView.QueryInterface(Ci.nsIInterfaceRequestor)

I think you can check event.target against docShell.contentViewer.DOMDocument. That's a little shorter.

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +332,5 @@
>    handleEvent: function(event) {
> +    let window = event.currentTarget;
> +    if (event.target != window.document) {
> +      // We use capturing listeners so we have precendence over content script
> +      // listeners, but only care about events targetted to the element we're

targeted?
Attachment #8687636 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/integration/fx-team/rev/8e712e56faf8ef12537ad604f38bca4f06665a7b
Bug 1215893: [webext] Check capturing event listeners for the correct target. r=billm

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8e712e56faf8
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment on attachment 8687636 [details] [diff] [review]
[webext] Check capturing event listeners for the correct target

Approval Request Comment
[Feature/regressing bug #]: Bug 1175770

[User impact if declined]:

Without this fix, attempting to load an <iframe>, and <img>, or anything else which fires a "load" event, in a background page causes an infinite loop, which makes the browser UI unresponsive and is difficult to break, even with a browser restart.

Given the current attention WebExtensions are receiving on Developer Edition, and the number of use cases for loading content into background pages this way, this is likely to be encountered by many developers, and give a negative impression of the stability of the framework.

[Describe test coverage new/current, TreeHerder]:

The tests for the background page API this most directly affects, and the panel APIs which it touches, are fairly extensive, and the patch adds sufficient additional tests to ensure that the fix is effective.

[Risks and why]: 

Low. This patch only affects the WebExtensions API, which developers should be aware is in a pre-release state. The changes are fairly minor, and well-tested, so the chance for regression is minimal.

[String/UUID change made/needed]: None.
Attachment #8687636 - Flags: approval-mozilla-aurora?

Comment 9

2 years ago
Comment on attachment 8687636 [details] [diff] [review]
[webext] Check capturing event listeners for the correct target

Been in Nightly for 10 days and impacts WebExtensions in DevEdition, taking it for Aurora44.
Attachment #8687636 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

2 years ago
status-firefox44: --- → affected

Updated

2 years ago
Iteration: --- → 45.2 - Nov 30
has problems during uplift 

grafting 315364:8e712e56faf8 "Bug 1215893: [webext] Check capturing event listeners for the correct target. r=billm"
merging browser/components/extensions/ext-utils.js
merging toolkit/components/extensions/Extension.jsm
merging toolkit/components/extensions/ExtensionContent.jsm
merging toolkit/components/extensions/test/mochitest/mochitest.ini
warning: conflicts while merging browser/components/extensions/ext-utils.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging toolkit/components/extensions/test/mochitest/mochitest.ini! (edit, then use 'hg resolve --mark')
Flags: needinfo?(kmaglione+bmo)
Created attachment 8695652 [details] [diff] [review]
Patch grafted onto aurora
Flags: needinfo?(kmaglione+bmo)
https://hg.mozilla.org/releases/mozilla-aurora/rev/bf8a7ca8ccdb
status-firefox44: affected → fixed

Comment 13

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/bf8a7ca8ccdb
status-b2g-v2.5: --- → fixed
You need to log in before you can comment on or make changes to this bug.