Closed Bug 1130275 Opened 5 years ago Closed 5 years ago

[EME] promise chaining is not done correctly in EME mochitests

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

Details

Attachments

(1 file)

Here is an example:

function dosomething(reason, shouldResolve) {
  return new Promise(function(resolve, reject) {
    console.log("starting:" + reason);
    if (shouldResolve) {
      console.log("resolving:" + reason);
      resolve();
    } else {
      console.log("rejecting:" + reason);
      reject();
    }
  });
}

function bail(reason) {
  return function() {
    console.log("bail:" + reason);
  };
}

dosomething("task1", false)

.then(function() {
  console.log("task1 resolved");
  return dosomething("task2", true);
}, bail("task1 rejected"))

.then(function() {
  console.log("task2 resolved");
  return dosomething("task3", true);
}, bail("task2 rejected"));

and the output:
"starting:task1"
"rejecting:task1"
"bail:task1 rejected"
"task2 resolved"
"starting:task3"
"resolving:task3"

Even task 1 is rejected, task 2 is still resolved which is not what we expect.

Due to that, our EME mochitests will go on even when some promise is rejected and produce strange error messages.
We should remove the reject function from then() as below to stop promise chaining properly:

function dosomething(reason, shouldResolve) {
  return new Promise(function(resolve, reject) {
    console.log("starting:" + reason);
    if (shouldResolve) {
      console.log("resolving:" + reason);
      resolve();
    } else {
      console.log("rejecting:" + reason);
      reject();
    }
  });
}

function bail(reason) {
  return function() {
    console.log("bail:" + reason);
  };
}

dosomething("task1", true)

.then(function() {
  console.log("task1 resolved");
  return dosomething("task2", false);
})

.then(function() {
  console.log("task2 resolved");
  return dosomething("task3", true);
});
Assignee: nobody → jwwang
Status: NEW → ASSIGNED
Comment on attachment 8561164 [details] [diff] [review]
1130275_proper_promise_chaining-v1.patch

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

::: dom/media/test/test_eme_persistent_sessions.html
@@ +105,5 @@
> +        .then(function(suceeded) {
> +          if (suceeded) {
> +            return Promise.resolve();
> +          } else {
> +            return Promise.reject("Fail to load recreatedSession, sessionId=" + sessionId);

We need to reject the promise when resolved value is false.

@@ -121,5 @@
>  
>          .then(function(suceeded) {
>            is(suceeded, false, token + " we expect the third session creation to fail, as the session should have been removed.");
>            manager.finished(token);
> -        }, bail(token + " failure to load session."));

We remove all bail("blah blah") from the rejection handler so when a promise is rejected in the middle of the chaining, we will skip all then()s until catch() is met.
Attachment #8561164 - Flags: review?(cpearce)
Comment on attachment 8561164 [details] [diff] [review]
1130275_proper_promise_chaining-v1.patch

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

Edwin can you please review this? Thanks.
Attachment #8561164 - Flags: review?(cpearce) → review?(edwin)
Comment on attachment 8561164 [details] [diff] [review]
1130275_proper_promise_chaining-v1.patch

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

::: dom/media/test/eme.js
@@ +258,5 @@
> +      }
> +      session.addEventListener("message", UpdateSessionFunc(test, token, sessionType));
> +      var p = session.generateRequest(ev.initDataType, ev.initData);
> +      var r = bail(token + ": session.generateRequest failed");
> +      return chain(p, r);

Log if we succeeded here as well.
Attachment #8561164 - Flags: review?(edwin) → review+
https://hg.mozilla.org/mozilla-central/rev/9b3ea7966b91
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.