1420716 - Race between insertCSS and webNavigation?

Status: RESOLVED INVALID

(WebExtensions :: Untriaged, defect)

Description

[Yuri Khan]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20171126100425

Steps to reproduce:

My goal is to inject custom CSS into each and every page as soon as it starts loading.

Trying to do it this way, in a background script of a WebExtension having permissions ["tabs", "webNavigation", "<all_urls>"]:

browser.webNavigation.onCommitted &&
browser.webNavigation.onCommitted.addListener(details => {
  console.log('onCommitted', details);
  return browser.tabs.insertCSS(details.tabId, {
    code: 'body { border: 10px solid red; }',
    frameId: details.frameId,
    runAt: 'document_start'
  })
  .then(() => console.log('injected for', details))
  .catch(e => console.log(details, e));
});

The complete source is at: https://github.com/yurikhan/test-firefox-webNavigation

1. Load the extension.
2. Open a new tab.
3. Load http://centaur.ath.cx/test-index.html
4. Reload it a few times.
5. Save the page to a local disk.
6. Open a new tab.
7. Load the page saved in step 5.
8. Reload it a few times.


Actual results:

When loaded over the network (steps 3–4), most of the time, the whole page has a red border but the content of the frame does not.

In the console:

Object { url: "http://centaur.ath.cx/test-frame.ht…", timeStamp: 1511704268208, frameId: 4294967423, parentFrameId: 0, tabId: 79, windowId: 3, transitionType: "auto_subframe", transitionQualifiers: Array[0] }

Error: Frame not found, or missing host permission

Stack trace:
Async*@moz-extension://926299b7-77f9-4efa-9b10-dd65905f7928/background.js:9:10
  background.js:15:15

When loaded from local disk (steps 7–8), both the main page and the frame contents have borders.


Expected results:

In both cases (steps 3–4 and 7–8), both the main page and frame contents should have borders.


Changing the code to run onDOMContentLoaded causes the style to apply reliably but there is a small time window after the page starts loading and displaying but before the style is applied (aka Flash of Unstyled Content). I would like to avoid that.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Yuri,

webNavigation.onCommitted is unfortunately too early to be able to inject a CSS reliably using tabs.insertCSS, as it is described on MDN (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webNavigation/onCommitted):

   Fired when a navigation is committed. At least part of the new document has been received 
   from the server and the browser has decided to switch to the new document.

And so, when this event is fired, the browser has decided that it will switch to the new document, but it is likely that it has not switched to the new document yet (nevertheless the document is loaded in a different process, and so if you try to run tabs.insertCSS on the related tab, there is a chance that the document has been already loaded, but no guarantees).

Like you describe in comment 0, webNAvigation.onDOMContentLoaded is the first webNavigation event that you can use to be sure that the browser has already switched to the new document, but given that the document is loading in a different document, there is an high chance that the document is already being rendered.

The most reliable way to inject a CSS while the document is still loading is a CSS contentScript registered through the manifest.json file (or using the new contentScripts.register API which have been introduced recently, and so it would be only available on Firefox >= 59).

I'm closing this issue as invalid because the described behavior is expected.

Comment 2

[Yuri Khan]

contentScripts.register sounds like just what I need. I can haz documentation?

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Yuri Khan from comment #2)
> contentScripts.register sounds like just what I need. I can haz
> documentation?

It has been landed recently and so it doesn't have docs on MDN yet, but it has been already marked as dev-doc-needed (which means that it has been already queued to be documented), in the meantime the easiest way to see how it can be used in an extension is looking at the existent test case and the API schema:

- https://searchfox.org/mozilla-central/rev/ba2b0cf4d16711d37d4bf4d267b187c9a27f6638/toolkit/components/extensions/test/xpcshell/test_ext_contentScripts_register.js#34-38,62
- https://searchfox.org/mozilla-central/source/toolkit/components/extensions/schemas/content_scripts.json

The API is pretty small: `contentScripts.register` returns a script object, which is an object that exposes a single `script.unregister()` method.

Comment 4

[Yuri Khan]

Hm. I tried it.

Good news: It does inject the style at the earliest.

Bad news: After the style is loaded into the page, I cannot remove it with ‘removeCSS’. Unregistering only affects the following page reloads. Which I’d like to avoid.

The functionality I’m trying to implement is:

* I have a collection of stylesheets. Each has an ‘enabled’ boolean flag and a match pattern.
* Enabling or disabling a stylesheet should inject (uninject) that stylesheet in all currently existing matching frames of all tabs.
* When a stylesheet is enabled and a matching page is loaded into any frame, the stylesheet should be injected as early as possible.

I understand the problem stems from treating CSS and JS uniformly. You can’t unexecute JS. However, CSS can be applied and removed at will, as demonstrated by ‘insertCSS’ and ‘removeCSS’.

Perhaps ‘removeCSS’ should be able to remove stylesheets injected as content scripts, registered either via ‘manifest.json’ or ‘contentScripts.register’?

Should I file a new bug?

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Yuri Khan from comment #4)
> I understand the problem stems from treating CSS and JS uniformly. You can’t
> unexecute JS. However, CSS can be applied and removed at will, as
> demonstrated by ‘insertCSS’ and ‘removeCSS’.
> 
> Perhaps ‘removeCSS’ should be able to remove stylesheets injected as content
> scripts, registered either via ‘manifest.json’ or ‘contentScripts.register’?
> 
> Should I file a new bug?

Yes, thanks! (and you can also CC me on the newly filed bug)

I think that it would make sense (I'm not sure yet if it should be the default behavior, but I definitely believe that it should be possible, and it could be an optional behavior triggered by passing an optional parameter to `script.unregister`).

Thank you very much for the useful feedback about it, very much appreciated.

Comment 6

[Yuri Khan]

Filed bug 1423323.