1271692 - do not fail network interception if fetch event handle throws

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently we fail the network request if the fetch event handler throws.  This is not what the spec says to do and not what chrome does.  There is a slight spec bug, but we should align with chrome here.

See:

https://github.com/slightlyoff/ServiceWorker/issues/896

Comment 1

[Tom Tung [:tt, :ttung]]

I'd like to work on this bug.

Comment 2

[Tom Tung [:tt, :ttung]]

Attachment: [WIP] Bug-1271692-P1-v1- Do not fail if fetch event, install event's handle throw.

Remove the code for calling ChannelCancelRunnable / SuppressException when throw the exception.

Comment 3

[Tom Tung [:tt, :ttung]]

Attachment: Bug-1271692-P1-v1.1- Do not fail if fetch event, install event's handle throw.

Hi Ben,

I think I need to ensure fetch/install event should not fail when event's handle throw. For instance, the following situations should not cause failure:
```
self.addEventListener('install', function(event) { throw new Error(); }
self.addEventListener('fetch', function(event) { throw new Error(); }
```

But, we still should stop install when throwing in the waitUntil ,like the following case:
```
self.addEventListener('install', function(event) { 
  event.waitUntil(new Promise(function(aRequest, aResponse) {
      throw new Error();
    }));
```

Thus, I wrote this patch, I removed the checking for exception flag in both fetch event and install event, but I'm not sure about whether I understand this issue correctly and implement in the right direction. 

I would like to make sure it right before I go far away, could you give me some feedback about this patch? Thanks! 

I'll fix all the other broken tests if I'm in the right direction.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8809788 [details] [diff] [review]
Bug-1271692-P1-v1.1- Do not fail if fetch event, install event's handle throw.

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

LGTM.  Thanks!

Comment 5

[Tom Tung [:tt, :ttung]]

Attachment: [WIP] Bug-1271692-P1-v1-2- Do not fail if event's handle throw.

This patch is for make event handle continue to work when catching an exception but still report error message. 

Current status:
- Remove CancelChannelRunnable when exception in FetchEventRunnable
- Modify DispatchExtendableEventOnWorkerScope() and make it to return nsresult since we need to report exception when exception happens.
- Modify few testcases to follow the issue.

Next step: 
- Try to fix registration-attribute.https.html. 

Applying this patch will make LifecycleEventWorkerRunnable::DispatchLifecycleEvent() catch an exception as running registration-attribute.https.html. 

I guess that the SW's status doesn't go as expected and it hit the assert which raise the exception flag. I'll try to figure out how to fix this test.

Comment 6

[Tom Tung [:tt, :ttung]]

Attachment: Bug-1271692-P1-v1.3- Do not fail if event's handle throw.

(In reply to Tom Tung [:tt] from comment #5)
I decided not to fix registration-attribute.https.html in this bug. I found that there are somethings need to be implemented if I want to fix registration-attribute.https.html. (e.g., we should fire 'updatefound' event before 'install' event but we didn't.) 

Hi Ben,

In these patch, I mainly remove the early return when the exception flag raise. To make each event can still report the error (but do not stop), I modified the return type of DispatchExtendableEventOnWorkerScope(). Besides, I fix the tests that excepted fail as event's handle throw. 

Could you help me to review this patch? Thanks!

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a08892641e44b041e32c3197a65104c7dad15cb6&selectedJob=31946114

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8814842 [details] [diff] [review]
Bug-1271692-P1-v1.3- Do not fail if event's handle throw.

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

r=me with comments addressed

::: dom/workers/ServiceWorkerPrivate.cpp
@@ +504,5 @@
>      }
>  
>      extendableEvent->SetTrusted(true);
>  
> +    return !NS_FAILED(DispatchExtendableEventOnWorkerScope(aCx,

s/!NS_FAILED/NS_SUCCEEDED/

@@ +772,5 @@
> +  nsresult rv = DispatchExtendableEventOnWorkerScope(aCx,
> +                                                     aWorkerPrivate->GlobalScope(),
> +                                                     event,
> +                                                     watcher);
> +  // Do not block event when throw an exception.

Maybe reword as "Do not fail event processing when an exception is thrown."

@@ +839,5 @@
>      WorkerPrivate* workerPrivate = mWorkerPrivate;
>      mWorkerPrivate->AssertIsOnWorkerThread();
> +    if (aNullWorkerPrivate) {
> +      mWorkerPrivate = nullptr;
> +    }

Why do you need this?  It doesn't seem like anything ever passes false for aNullWorkerPrivate.

If we do need to avoid nulling the pointer we can actually just remove the nullptr assignment completely.  I'm not sure why it was ever done.  We don't check if its nullptr anywhere in PushErrorReporter, so setting it nullptr isn't detectable by the running code today and its not creating any safety checks.

Comment 8

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #7)

Thanks for the review! I'll address the comment!

> Comment on attachment 8814842 [details] [diff] [review]
> Bug-1271692-P1-v1.3- Do not fail if event's handle throw.
> 
> Review of attachment 8814842 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed
> 
> ::: dom/workers/ServiceWorkerPrivate.cpp
> @@ +839,5 @@
> >      WorkerPrivate* workerPrivate = mWorkerPrivate;
> >      mWorkerPrivate->AssertIsOnWorkerThread();
> > +    if (aNullWorkerPrivate) {
> > +      mWorkerPrivate = nullptr;
> > +    }
> 
> Why do you need this?  It doesn't seem like anything ever passes false for
> aNullWorkerPrivate.

I do this to avoid hitting assertion for checking WorkPrivate.
In original case, when exception flag is raised, we will call Report() function which null the mWorkerPrivate. That will hit assertion because some functions here check aWorkerPrivate in the top of function.
 
> If we do need to avoid nulling the pointer we can actually just remove the
> nullptr assignment completely.  I'm not sure why it was ever done.  We don't
> check if its nullptr anywhere in PushErrorReporter, so setting it nullptr
> isn't detectable by the running code today and its not creating any safety
> checks.

Oh, I see! Thanks for the explanation!

Comment 9

[Tom Tung [:tt, :ttung]]

Attachment: [Final] Bug-1271692-P1-v2- Do not fail event processing if an exception is thrown in ServiceWorker. r=bkelly.

Address comment and rebase patch.

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

Comment 10

[Tom Tung [:tt, :ttung]]

Sorry, I forget to modify the patch name

Comment 11

[Tom Tung [:tt, :ttung]]

Attachment: [Final] Bug-1271692-P1-v2- Do not fail event processing if an exception is thrown in ServiceWorker. r=bkelly.

Commit message updated. :)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/362a85a19937
Do not fail event processing if an exception is thrown in ServiceWorker. r=bkelly

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/362a85a19937

Comment 14

[Tom Tung [:tt, :ttung]]

Sorry for forgetting to test the second comment in github, and it caused crash which decribed in bug 1342255. 
I think I did it, but obviously, I didn't. :(  
I'll pay more attention to similar things.

Comment 15

[Mike Kaply [:mkaply]]

Twitter links on mobile were broken in Firefox 52 because of this.

Is this a larger problem that should be fixed in the 52 ESR as well?

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Mike Kaply [:mkaply] from comment #15)
> Twitter links on mobile were broken in Firefox 52 because of this.

I think you mean it was fixed by this.

> Is this a larger problem that should be fixed in the 52 ESR as well?

Service workers are disabled in FF52 ESR.