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

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Kris Maglione [:kmag]]

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

[Shivam]

Hey i would like to work on this bug !! Can u please help me how to go about it

Comment 2

[Kris Maglione [:kmag]]

(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

Comment 3

[Kris Maglione [:kmag]]

I'm just going to retarget this bug, since we do this in several places.

Comment 4

[Kris Maglione [:kmag]]

Attachment: [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.

Comment 5

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

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?

Comment 6

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/8e712e56faf8ef12537ad604f38bca4f06665a7b
Bug 1215893: [webext] Check capturing event listeners for the correct target. r=billm

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8e712e56faf8

Comment 8

[Kris Maglione [:kmag]]

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.

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 10

[Carsten Book [:Tomcat]]

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')

Comment 11

[Kris Maglione [:kmag]]

Attachment: Patch grafted onto aurora

Comment 12

[Kris Maglione [:kmag]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bf8a7ca8ccdb

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/bf8a7ca8ccdb