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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

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);
});

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1130275_proper_promise_chaining-v1.patch

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Try is green: https://tbpl.mozilla.org/?tree=Try&rev=8e962d3ef0d6

Comment 6

[Edwin Flores [inactive from 2016-12-01] [:eflores] [: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.

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Try is green: https://tbpl.mozilla.org/?tree=Try&rev=8e962d3ef0d6

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/9b3ea7966b91

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9b3ea7966b91

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/195d0c8e3c41