Closed Bug 1384041 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Session Restore, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: bevis, Assigned: bevis)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Just FYI, bug 1383328 will expose labeled event targets to JS.
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!
Assignee: nobody → btseng
Flags: needinfo?(wiwang)
sorry to set ni unexpectedly.
Flags: needinfo?(wiwang)
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.
Depends on: 1383328
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
Attachment #8891909 - Flags: review?(wmccloskey)
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.
Attachment #8891909 - Attachment is obsolete: true
Attachment #8891909 - Flags: review?(wmccloskey)
Attachment #8891951 - Flags: review?(wmccloskey)
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.
Attachment #8891951 - Flags: review?(wmccloskey) → review+
Pushed by btseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef75a524b850
Label the use of setTimeout of Timer.jsm in content-sessionStore.js. r=billm
https://hg.mozilla.org/mozilla-central/rev/ef75a524b850
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.