596018 - switch contentScriptWhen="start" from content-document-global-created to document-element-inserted

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Myk Melez [:myk] [@mykmelez]]

When the value of contentScriptWhen is "start", content scripts are loaded into a content symbiont when the content-document-global-created notification is received.  But that notification is too early to load jQuery and other JS libraries that assume the presence of the document element, because the document element has not yet been created.

Now that bug 579764 has been fixed, we should modify ContentSymbiont (or whatever replaces it per bug 588737) to load content scripts when the document-element-inserted notification is received instead.

Note that Gecko 1.9.2 doesn't dispatch the document-element-inserted notification, so making this change requires us to either drop support for Gecko 1.9.2 or branch based on the Gecko version.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: implements change

Here's a patch that implements the change.  I think this is good to go, except that it exposes bug 596666 reproducibly.

Note: because about:blank doesn't have a document element, it doesn't trigger document-element-inserted.  That's ok, as consumers that really want to load a content script for about:blank can do so on "ready" rather than "start" (for about:blank, the two are effectively the same).

But tests that load a content script for about:blank need to be updated to either load a URL that does trigger document-element-inserted (like about:buildconfig) or load on "ready".  This patch makes those changes.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: unrotted, and with test

This patch applies to the tip, includes a test that confirms the document element is present on 4.0b6+, and resolves a bug in the symbiont destructor.

This is now good to go.

Comment 4

[Dietrich Ayala (:dietrich)]

Comment on attachment 476946 [details] [diff] [review]
patch v2: unrotted, and with test


>+const HAS_DOCUMENT_ELEMENT_INSERTED =
>+        xulApp.versionInRange(xulApp.platformVersion, "2.0b6", "*"),
>+      ON_START = HAS_DOCUMENT_ELEMENT_INSERTED ? 'document-element-inserted' :
>+                 'content-document-global-created',
>+      ON_READY = 'DOMContentLoaded';

nit: readability suffers here, for no great win. can you make them independent lines?

r=me otherwise!

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Myk there is one thing that this patch trying to solve that is solved already:

+    // The frame might not have been initialized yet.
+    if (!this._frame)
+      return;

This issue actually happened on unload because of a bug:

http://github.com/Gozala/jetpack-sdk/commit/4ee1d49137c1bccb5f79a80a2a54dd20ddf492a9

I have submitted patch for a review that will solve that:

https://bugzilla.mozilla.org/show_bug.cgi?id=598137

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Actually I was wrong so just ignore my last comment.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #4)
> nit: readability suffers here, for no great win. can you make them independent
> lines?

Yup, done (in both such sections of multiple constants).

Also fixed trivial conflicts from recent checkins.

Fixed by changeset https://hg.mozilla.org/labs/jetpack-sdk/rev/ed8e394031c9.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".