Use right value for Resolve Job Promise #1054
Conversation
|
@mkruisselbrink, from #829 (comment),
This was done in other patches already. This PR addresses using the right value for resolving the equivalent jobs' promises. Please review. |
|
It kind of feels extremely pedantic (and specs in general seem to do a terribly job at specifying what Realm objects are created in), but this doesn't quite seem correct yet (other than "the value that corresponds to value" not seeming very well defined). In particular at the places where Resolve Job Promise is called we currently call it with a Maybe the right fix would be to pass the service worker registration itself, rather than a javasript object associated with it to Resolve Job Promise, and then have the tasks that are queued by Resolve Job Promise create (or lookup? I'm not sure from reading the current spec if we're supposed to always return the same instance for the same registration in the same realm, or if a different instance can be returned sometimes) the correct instance to resolve the promise with. Not sure how we'd then deal with the one job type that doesn't resolve with a registration, but instead with a boolean. And again all of this feels very pedantic and doesn't seem to be something that most specs get right either (probably because actually getting it right is so tricky/complicated). Also to be truly pedantic, Reject Job Promise should probably make it explicit that all the different promises get rejected with different instances of the same type of exception. As currently spec'ed the following test would pass, which I don't think is desired: let p1 = navigator.serviceWorker.register("http://example.com/foo.js");
let p2 = navigator.serviceWorker.register("http://example.com/foo.js");
p1.catch(e => e.custom = "bla");
p2.catch(e => assert_equals(e.custom, "bla"));Maybe the easiest fix is to completely get rid of the Resolve Job Promise and Reject Job Promise algorithms, but that of course would make all the algorithms currently using them more complicated, which would also suck. |
This changes Resolve Job Promise/Reject Job Promise algorithms and their call sites to use correct fulfillment values/rejection reasons for equivalent job promises by specifying the associated realms. Fixes #829.
|
Thanks to whatwg/html@58879aa, I tried to address it again. @mkruisselbrink, @domenic, please take a look. |
|
Sorry for losing the commit history for @mkruisselbrink's previous review. |
docs/index.bs
Outdated
| 1. If |job|'s [=job/client=] is not null, <a>queue a task</a> to reject |job|'s [=job/job promise=] with |reason| on |job|'s [=job/client=]'s <a>responsible event loop</a> using the <a>DOM manipulation task source</a> as the <a>task source</a>. | ||
| 1. For each |equivalentJob| in |job|'s <a>list of equivalent jobs</a>: | ||
| 1. If |equivalentJob|'s [=job/client=] is not null, <a>queue a task</a> to reject |equivalentJob|'s [=job/job promise=] with |reason| on |equivalentJob|'s [=job/client=]'s <a>responsible event loop</a> using the <a>DOM manipulation task source</a> as the <a>task source</a>. | ||
| 1. If |job|'s [=job/client=] is not null, [=queue a task=], on |job|'s [=job/client=]'s [=responsible event loop=] using the [=DOM manipulation task source=], to reject |job|'s [=job/job promise=] with an exception that is a copy of |reason|, in |job|'s [=job/client=]'s [=environment settings object/Realm=]. |
There was a problem hiding this comment.
"A copy of" is a bit imprecise, but it's probably OK. I guess the truly rigorous way to do this would be to just have the reason be the type of the exception (plus maybe any data necessary to construct a good message string, but that's kind of out of scope for the spec). Then you actually create the exception object during the queued task.
This assumes reason is only ever UA-created. If it is user-code-created, then we have a bigger problem.
There was a problem hiding this comment.
Good point. I looked up and tried to use the WebIDL definitions in the follow-up commit. I referenced https://heycam.github.io/webidl/#es-creating-throwing-exceptions to create an exception. In that algorithm, it uses the current Realm to construct an object. So, I didn't put the target realm in the step within the queued task. Please take a look.
| 1. For each |equivalentJob| in |job|'s <a>list of equivalent jobs</a>: | ||
| 1. If |equivalentJob|'s [=job/client=] is not null, <a>queue a task</a> to resolve |equivalentJob|'s [=job/job promise=] with |value| on |equivalentJob|'s [=job/client=]'s <a>responsible event loop</a> using the <a>DOM manipulation task source</a> as the <a>task source</a>. | ||
| 1. If |equivalentJob|'s [=job/client=] is null, continue to the next iteration of the loop. |
There was a problem hiding this comment.
Feel free to use [=iteration/continue=] instead of "continue to the next iteration of the loop"
There was a problem hiding this comment.
Addressed. I'll change the other places in a separate patch.
docs/index.bs
Outdated
| 1. If |equivalentJob|'s [=job/client=] is null, continue to the next iteration of the loop: | ||
| 1. [=Queue a task=], on |equivalentJob|'s [=job/client=]'s [=responsible event loop=] using the [=DOM manipulation task source=], to reject |equivalentJob|'s [=job/job promise=] with an exception that is a copy of |reason|, in |equivalentJob|'s [=job/client=]'s [=environment settings object/Realm=]. | ||
| 1. If |equivalentJob|'s [=job/client=] is null, [=iteration/continue=]. | ||
| 1. [=Queue a task=], on |equivalentJob|'s [=job/client=]'s [=responsible event loop=] using the [=DOM manipulation task source=], to reject |equivalentJob|'s [=job/job promise=] with a [=exception/create|new=] [=exception=] with |errorName| and a user agent-defined [=exception/message=]. |
There was a problem hiding this comment.
This structure doesn't quite work, because you need to say "DOMException" for the "SecurityError" cases, and not say DOMException for the TypeError case.
I'd maybe make the argument "errorData, the information necessary to [=exception/create|create an exception=]". Then you could say "with job and "SecurityError" DOMException", or "with job and TypeError".
Also, you should state the realm. Inside a queued task, you've "lost" the current realm, since current realm refers to that of the executing function object.
There was a problem hiding this comment.
Right. That structure didn't cover both cases. I tried it again on your comments. I left the "user agent-defined message" bit as-is. Would it make sense?
There was a problem hiding this comment.
Thanks for your review!
This changes Resolve Job Promise algorithm to use a right value for
equivalent job promises by specifying the value should be the one for
its relevant Realm.
Fixes #829.