Background page scripts are executed after DOMContentLoaded and load events

RESOLVED FIXED in Firefox 50

Status

defect
P4
normal
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: frolovm, Assigned: robwu)

Tracking

45 Branch
mozilla50
x86_64
Windows 10
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions -

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: triaged)

Attachments

(2 attachments)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/47.0.2526.106 Safari/537.36

Steps to reproduce:

I have a background page with the following code:

window.addEventListener('DOMContentLoaded', ...);



Actual results:

The event is never fired while the same works in Google Chrome.



Expected results:

The event should fire.
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
A testcase would be useful. Do you have a background page, or a list of background scripts, declared in your manifest? The latter, we create in response to the load event, so those scripts are not loaded until after DOMContentLoaded has fired.
I have a single background script listed int he manifest.  Sounds like this is by design.

However, this is not consistent with Google Chrome implementation where background scripts are loaded before DOMContentLoaded is fired.

I have already worked around this by checking for document.readyState
It may not be consistent, but the behavior you're relying on is undocumented, and therefore shouldn't be relied on.

The only real way I see around this is to pre-generate the background page HTML, including script nodes, rather than injecting the script nodes dynamically. Well, that, or get rid of the script nodes automatically, and load the scripts synchronously using the subscript loader, but that may cause other problems.

I'd rather not do that unless this turns out to affect a large number of add-ons, though.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-webextensions?
Summary: DOMContentLoaded not fired → Background page scripts are executed after DOMContentLoaded and load events
Priority: -- → P4
Whiteboard: triaged
Flags: blocking-webextensions? → blocking-webextensions-
Duplicate of this bug: 1286057
(In reply to Kris Maglione [:kmag] from comment #3)
> It may not be consistent, but the behavior you're relying on is
> undocumented, and therefore shouldn't be relied on.
> 
> The only real way I see around this is to pre-generate the background page
> HTML, including script nodes, rather than injecting the script nodes
> dynamically. Well, that, or get rid of the script nodes automatically, and
> load the scripts synchronously using the subscript loader, but that may
> cause other problems.
> 
> I'd rather not do that unless this turns out to affect a large number of
> add-ons, though.

This is a better approach IMO. Doing so solves the two use cases from bug 1286057.
Assignee: nobody → rob
- Fixes bugzil.la/1234677
- Fixes bugzil.la/1286057
- Fixes bug: the URL failed to load if a query string or reference
  fragment was present.

Review commit: https://reviewboard.mozilla.org/r/63770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63770/
Attachment #8770297 - Flags: review?(wmccloskey)
Status: NEW → ASSIGNED
Attachment #8770297 - Flags: review?(wmccloskey) → review+
Comment on attachment 8770297 [details]
Bug 1234677 - Introduce _generated_background_page.html

https://reviewboard.mozilla.org/r/63770/#review60848

This looks good!

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:89
(Diff revision 1)
> +                                              nsACString& aResult)
> +{
> +  // Create special moz-extension:-pages such as moz-extension://foo/_blank.html
> +  // for all registered extensions. We can't just do this as a substitution
> +  // because substitutions can only match on host.
> +  if (SubstitutingProtocolHandler::HasSubstitution(aHost)) {

If you do this as:

if (SubstitutingProtocolHandler::HasSubstitution(aHost)) {
  return false;
}

then the rest can be indented less.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:90
(Diff revision 1)
> +    int32_t pathend = aPath.FindChar('?');
> +    if (pathend == -1) {
> +      pathend = aPath.FindChar('#');
> +      if (pathend == -1) {
> +        pathend = aPath.Length();
> +      }
> +    }

Rather than doing this, perhaps we should change the caller to avoid passing in the ? and # portion.

http://searchfox.org/mozilla-central/source/netwerk/protocol/res/SubstitutingProtocolHandler.cpp#353

This code could instead do something like this:

nsCOMPtr<nsIURL> url = do_QueryInterface(uri);
rv = url->GetFilePath(path);
if (NS_FAILED(rv)) return rv;

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:103
(Diff revision 1)
> +    if (pathname.EqualsLiteral("/_blank.html")) {
> +      aResult.AssignLiteral("about:blank");
> +      return true;
> +    }
> +    if (pathname.EqualsLiteral("/_generated_background_page.html")) {
> +      aResult.AssignLiteral("data:text/html;charset=utf-8,");

This doesn't seem useful.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:106
(Diff revision 1)
> +    }
> +    if (pathname.EqualsLiteral("/_generated_background_page.html")) {
> +      aResult.AssignLiteral("data:text/html;charset=utf-8,");
> +      nsCOMPtr<nsIAddonPolicyService> aps =
> +        do_GetService("@mozilla.org/addons/policy-service;1");
> +      if (aps) {

If aps is null, let's just return false here.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:110
(Diff revision 1)
> +        do_GetService("@mozilla.org/addons/policy-service;1");
> +      if (aps) {
> +        nsresult rv = aps->GetGeneratedBackgroundPageUrl(aHost, aResult);
> +        NS_ENSURE_SUCCESS(rv, false);
> +        if (!aResult.IsEmpty()) {
> +          MOZ_ASSERT(aResult.Find("data:") == 0);

Please use MOZ_RELEASE_ASSERT so we crash in opt builds. This should be used for all non-performance-sensitive assertions.

::: toolkit/components/extensions/test/mochitest/test_ext_background_generated_url.html:20
(Diff revision 1)
> +
> +add_task(function* test_url_of_generated_background_page() {
> +  function backgroundScript() {
> +    const EXPECTED_URL = browser.runtime.getURL("/_generated_background_page.html");
> +    browser.test.assertEq(EXPECTED_URL, location.href);
> +    browser.test.sendMessage("script done", EXPECTED_URL);

browser.test.notifyPass("test-name") / extension.awaitFinish("test-name") is the idiomatic way to do this.
Comment on attachment 8770297 [details]
Bug 1234677 - Introduce _generated_background_page.html

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63770/diff/1-2/
https://reviewboard.mozilla.org/r/63770/#review60848

> Rather than doing this, perhaps we should change the caller to avoid passing in the ? and # portion.
> 
> http://searchfox.org/mozilla-central/source/netwerk/protocol/res/SubstitutingProtocolHandler.cpp#353
> 
> This code could instead do something like this:
> 
> nsCOMPtr<nsIURL> url = do_QueryInterface(uri);
> rv = url->GetFilePath(path);
> if (NS_FAILED(rv)) return rv;

`path` is also used in the resource:-handler (http://searchfox.org/mozilla-central/rev/c44d0b1673fef5e0e2e19fa82d6780a74c186151/netwerk/protocol/res/nsResProtocolHandler.cpp#77-88), so I can't change it like that.

> browser.test.notifyPass("test-name") / extension.awaitFinish("test-name") is the idiomatic way to do this.

The passed URL is actually used in the test, to open a new tab with the same URL. I have however replaced another occurrence of sendMessage with notifyPass following your recommendation.
Comment on attachment 8770297 [details]
Bug 1234677 - Introduce _generated_background_page.html

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63770/diff/2-3/
Comment on attachment 8770297 [details]
Bug 1234677 - Introduce _generated_background_page.html

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63770/diff/3-4/
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4dd2466573ec
Introduce _generated_background_page.html r=billm
Sorry had to back out due to Mochitest test_chrome_ext_background_page.html failure, e.g., https://treeherder.mozilla.org/logviewer.html#?job_id=766792&repo=autoland#L15861
Flags: needinfo?(wmccloskey)
Flags: needinfo?(rob)
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/77e960d65f64
Backed out changeset 4dd2466573ec for Mochitest test_chrome_ext_background_page.html failure
A nice extra effect of this is that it will also work after reloading
the background page (location.reload()) and it should also work for
child frames in background pages.

Review commit: https://reviewboard.mozilla.org/r/65492/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65492/
Attachment #8772722 - Flags: review?(wmccloskey)
Comment on attachment 8770297 [details]
Bug 1234677 - Introduce _generated_background_page.html

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63770/diff/4-5/
Added a new patch in comment 15, to be landed before the original patch. The test failed because it runs a dialog in the background script; originally background scripts were run somewhere around the onload event, now they are run before that, so the current alert-overwriting wizardry kicks in too late (at window.onload).
Flags: needinfo?(rob)
Comment on attachment 8772722 [details]
Bug 1234677 - Overwrite alert at window init instead of onload

https://reviewboard.mozilla.org/r/65492/#review62634
Attachment #8772722 - Flags: review?(wmccloskey) → review+
Flags: needinfo?(wmccloskey)
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/58a8eeabbe7f
Overwrite alert at window init instead of onload r=billm
https://hg.mozilla.org/integration/autoland/rev/36b524e69dc5
Introduce _generated_background_page.html r=billm
https://hg.mozilla.org/mozilla-central/rev/58a8eeabbe7f
https://hg.mozilla.org/mozilla-central/rev/36b524e69dc5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Blocks: 1219229
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.