1234677 - Background page scripts are executed after DOMContentLoaded and load events

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P4)

Description

[Michail Frolov]

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.

Comment 1

[Kris Maglione [:kmag]]

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.

Comment 2

[Michail Frolov]

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

Comment 3

[Kris Maglione [:kmag]]

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.

Comment 5

[Rob Wu [:robwu]]

(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.

Comment 6

[Rob Wu [:robwu]]

Attachment: Bug 1234677 - Introduce _generated_background_page.html

- 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/

Comment 7

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

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 8

[Rob Wu [:robwu]]

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/

Comment 9

[Rob Wu [:robwu]]

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 10

[Rob Wu [:robwu]]

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 11

[Rob Wu [:robwu]]

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/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4dd2466573ec
Introduce _generated_background_page.html r=billm

Comment 13

[Iris Hsiao [:ihsiao]]

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

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77e960d65f64
Backed out changeset 4dd2466573ec for Mochitest test_chrome_ext_background_page.html failure

Comment 15

[Rob Wu [:robwu]]

Attachment: Bug 1234677 - Overwrite alert at window init instead of onload

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/

Comment 16

[Rob Wu [:robwu]]

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/

Comment 17

[Rob Wu [:robwu]]

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

Comment 18

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

Comment on attachment 8772722 [details]
Bug 1234677 - Overwrite alert at window init instead of onload

https://reviewboard.mozilla.org/r/65492/#review62634

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/58a8eeabbe7f
https://hg.mozilla.org/mozilla-central/rev/36b524e69dc5