tab.readyState uses CPOWs

RESOLVED FIXED in Firefox 46

Status

Add-on SDK
General
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mossop, Assigned: rpl)

Tracking

unspecified
mozilla46
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
Blocks: 1004745, 821779
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
Yes let's deprecate this.

The api should be separated if we need it anyhow.

Updated

3 years ago
Assignee: nobody → luca.greco

Comment 2

3 years ago
I found 46 add-ons using this in MXR.
(Assignee)

Comment 3

3 years ago
Created attachment 8696394 [details] [diff] [review]
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
Attachment #8696394 - Flags: feedback?(dtownsend)
(Reporter)

Comment 4

3 years ago
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.
Attachment #8696394 - Flags: feedback?(dtownsend) → feedback-
(Assignee)

Comment 5

3 years ago
Created attachment 8698115 [details] [diff] [review]
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)
Attachment #8696394 - Attachment is obsolete: true
Attachment #8698115 - Flags: review?(dtownsend)
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 6

3 years ago
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"
Attachment #8698115 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 7

3 years ago
Created attachment 8700018 [details] [diff] [review]
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
Attachment #8698115 - Attachment is obsolete: true
Attachment #8700018 - Flags: review+
(Assignee)

Comment 8

3 years ago
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
Keywords: checkin-needed

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b76ea647debc
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.