1384041 - Label the use of setTimeout of Timer.jsm in content-sessionStore.js

Status: RESOLVED FIXED

(Firefox :: Session Restore, enhancement)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

According to the analysis in bug 1382172 comment 9, the usage of ThrottleTimeoutsCallback is high. We need this to be labeled to gain more benefit from qDOM scheduler.

There is one thing that block this labeling is to expose proper SchedulerGroup event target to content-sessionStore.js then we can pass this event target to Timer.jsm to set before initiating the timer.

Comment 1

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

Just FYI, bug 1383328 will expose labeled event targets to JS.

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

After further discussion with :wiwang who is working on content-sessionStore.js recently, to label these timercallbacks in content script, we can just replace the use of setTimeout() provided from Timer.jsm with content.setTimeout() in which the callback will be labeled internally by the TimeoutManager of the content window.

I'll take this bug to follow up!

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

sorry to set ni unexpectedly.

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

Some failures captured in the tests of devtools if using content.setTimeout():
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1efbdbf11b558ede20a0ef1843ed1d1f203a30bd&selectedJob=118870652
It might be related to the throttling in the TimeoutManager internally.
The alternative solution is to pass tabEventTarget to the setTimeout in Timer.jsm.

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Patch: Label the use of setTimeout of Timer.jsm in content-sessionStore.js.

1. Define set{Timeout|Internal}WithTarget in Timer.jsm and give them names.
3. Labeling the use of setTimeout in content-sessionStore.js using tabEventTarget.

Treeherder result looks fine, btw,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=596041f4d44fd83c62ca82a84c60af4c192db11d

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Patch: Label the use of setTimeout of Timer.jsm in content-sessionStore.js

1. Merge the logic of setTimeout()/setInternal() into _setTimeoutOrIsInterval().
2. Define set{Timeout|Internal}WithTarget() in Timer.jsm and give them names.
3. Label the use of setTimeout in content-sessionStore.js using tabEventTarget.

Comment 7

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

Comment on attachment 8891951 [details] [diff] [review]
(v2) Patch: Label the use of setTimeout of Timer.jsm in content-sessionStore.js

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

::: toolkit/modules/Timer.jsm
@@ +32,5 @@
> +    timer.target = aTarget;
> +  }
> +
> +  let callback = {
> +      QueryInterface(iid) {

Two-space indent please. Also, it's easier to use XPCOMUtils.generateQI for this.

@@ +61,5 @@
>    gTimerTable.set(id, timer);
>    return id;
>  }
>  
> +this.setTimeout = function setTimeout(aCallback, aMilliseconds) {

Please add |...args| for these (i.e., rest parameters). Using |arguments| is slow and old-fashioned.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef75a524b850
Label the use of setTimeout of Timer.jsm in content-sessionStore.js. r=billm

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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