All users were logged out of Bugzilla on October 13th, 2018

Renable RequestSync tests in b2g

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8648416 [details] [diff] [review]
rs.patch
Attachment #8648416 - Flags: review?(nsm.nikhil)
Comment on attachment 8648416 [details] [diff] [review]
rs.patch

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

::: dom/requestsync/RequestSyncService.jsm
@@ +75,5 @@
>  
>    _timers: {},
>    _pendingRequests: {},
>  
> +  _afterSchedulingTasks: [],

Can you add a comment explaining the use of this? Particularly, callbacks passed to executeAfterScheduling get immediately called if there is no scheduling happening currently. Which functions should be wrapped into executeAfterScheduling() if new code is added?

@@ +104,3 @@
>          }
>        }
> +    }.bind(this),

Nit: Fat arrow function?

@@ +130,4 @@
>      this.forEachRegistration(function(aObj) {
> +      let key = this.principalToKey(aObj.principal);
> +      this.removeRegistrationInternal(aObj.data.task, key);
> +    }.bind(this));

Nit: same here.

@@ +655,5 @@
> +
> +    this.scheduling = true;
> +
> +    this.createTimer(aObj, function() {
> +      this.scheduling = false;

Important bug: createTimer() calls this callback only if the alarm service successfully adds the timer. If that fails, createTimer() isn't passing on a error callback which must reset this.scheduling (and maybe call the _afterSchedulingTasks?)

@@ +785,2 @@
>      this.updateObjectInDB(this._activeTask, function() {
>        // SchedulerTimer creates a timer and a nsITimer cannot be cloned. This

Considering this uses AlarmService, could you update the comment if it is still relevant.
Attachment #8648416 - Flags: review?(nsm.nikhil)
(Assignee)

Comment 4

3 years ago
Created attachment 8649161 [details] [diff] [review]
rs.patch
Attachment #8648416 - Attachment is obsolete: true
Attachment #8649161 - Flags: review?(nsm.nikhil)
Comment on attachment 8649161 [details] [diff] [review]
rs.patch

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

::: dom/requestsync/RequestSyncService.jsm
@@ +76,5 @@
>    _timers: {},
>    _pendingRequests: {},
>  
> +  // This array contains functions to be executed after the scheduling of the
> +  // curren task or immediately if there are not scheduling in progress.

Nit: 'current' and s/are not/is no

@@ +651,5 @@
>        return;
>      }
>  
> +    if (this.scheduling) {
> +      dump("ERROR!! RequestSyncService - ScheduleTimer called into ScheduleTimer.\n");

Do you want to abort here?
Attachment #8649161 - Flags: review?(nsm.nikhil) → review+
(Assignee)

Comment 6

3 years ago
Created attachment 8649506 [details] [diff] [review]
rs.patch
Attachment #8649161 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7936d101d4f5
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.