1194978 - Renable RequestSync tests in b2g

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: rs.patch

Comment 2

[Andrea Marchesini [:baku]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4307c18397dd

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: rs.patch

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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?

Comment 6

[Andrea Marchesini [:baku]]

Attachment: rs.patch

Comment 7

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7936d101d4f5

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/7936d101d4f5