1146606 - tab.readyState uses CPOWs

Status: RESOLVED FIXED

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

Description

[Dave Townsend [:mossop]]

This API uses a CPOW to synchronously get the document's readyState. Not sure how we can solve this, we could constantly message the readyState up to the parent whenever it changes in the child, the downside is that by the time it reaches the parent it might have changed already. Even with the current CPOW usage the readyState can be changing beneath us.

Probably the better choice is to just deprecate this, fix internal callers and have add-on developers use tab.attach if they want to monitor readyState, then they will know it is all async.

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

Yes let's deprecate this.

The api should be separated if we need it anyhow.

Comment 2

[Andy McKay]

I found 46 add-ons using this in MXR.

Comment 3

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

Attachment: 0001-Bug-1146606-forward-and-cache-remote-readyState-upda.patch

In this patch I'm trying the following approach:

- reuse the existent "tab-events.js" to forward readyState updates from the frame script to the addon running in the parent process
- use the "-document-global-created" to forward the "loading" state (and keep the number of subscribed listeners low, two more listeners per frame script)
- reuse the "-document-interactive" and "-document-loaded" to forward the "interactive" and "complete" states
- in the "tab-firefox" module, two Symbols are used to cache the remote readyState value in a hidden tab property and expose an hidden update method to update the cached value from the
  listener of the frame events

How it looks to you this kind of approach?

Follows a try run on the path:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fc44c5484e7

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 8696394 [details] [diff] [review]
0001-Bug-1146606-forward-and-cache-remote-readyState-upda.patch

Review of attachment 8696394 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is going to be broken in the case where an add-on is started after tabs already exist. With no events coming from those already loaded tabs we'll assume they are uninitialized rather than complete. We could have the tab-events script send up the readyState of every tab when first loaded but that will still leave a short race condition where an add-on might get a tabs object early with an incorrect readyState. That might be acceptable enough. The alternative I guess would need doing this at the browser.xml level and cache readyState there the same as we do for the contentURL but I don't think we want to get into the habit of doing that.

::: addon-sdk/source/lib/sdk/content/tab-events.js
@@ +21,5 @@
>  function topicListener({ subject, type }) {
> +  // NOTE detect the window from the subject:
> +  // - on *-global-created the subject is the window
> +  // - in the other cases it is the document object
> +  let window = subject.navigator ? subject : subject.defaultView;

We should use an instanceof Ci.nsIDOMWindow here rather than ducktyping

::: addon-sdk/source/lib/sdk/tabs/tab-firefox.js
@@ +45,5 @@
>  }
>  
> +// private tab setter used to set the cached value from the tabEvelListener
> +// on the tab object
> +const setRemoteReadyStateCached = Symbol("setRemoteReadyStateCached");

The set method doesn't seem to be needed really, you can just set the property directly.

Comment 5

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

Attachment: 0001-Bug-1146606-forward-and-cache-remote-readyState-upda.patch

This patch contains tweaks related to the following feedback comments:

- forward existent frames' document.readyState on tab-events.js loading
- check if a subject is a window using "instanceof Ci.nsIDOMWindow"
- remove the not needed hidden setter

and add a new test case for the "tab.readyState for existent tabs" scenario
(which fails if we do not forward the existent frames' document.readyState
when tab-events.js is loading in the content process).

Follows a try push which is running on this new patch version:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=d18f140249af

As described in the feedback, this patch still leaves a short race condition where an add-on might get a tabs object during the add-on loading phase (and the tab.readyState will be "uninitialized" because the cached value is still undefined)

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 8698115 [details] [diff] [review]
0001-Bug-1146606-forward-and-cache-remote-readyState-upda.patch

Review of attachment 8698115 [details] [diff] [review]:
-----------------------------------------------------------------

One minor change but this looks good to me!

::: addon-sdk/source/lib/sdk/content/tab-events.js
@@ +53,5 @@
> +
> +// Forward the existent frames's readyState.
> +for (let frame of frames) {
> +  let readyState = frame.content.document.readyState;
> +  frame.port.emit('sdk/tab/event', 'detected-existent-tab', { readyState });

I think I'd prefer to keep this as a single word like the other events we're sending up. Just call it "init"

Comment 7

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

Attachment: 0001-Bug-1146606-forward-and-cache-remote-readyState-upda.patch

Updated version of the previous patch with the suggested tweaks applies ('init' is definitely a better name for the new event)

The patch is now rebased on an updated tip of fx-team and I added the reviewer to the commit message.

I'm re-applying r+ as it is the same patch already reviewed with just the small tweak applied on it.

Try build:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1dab49f2aec

Comment 8

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

The last attached try push got stuck during the build.

Follows a completed try build on the same patch:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a9643dc51d1

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b76ea647debc

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b76ea647debc