do not fail network interception if fetch event handle throws

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: bkelly, Assigned: tt)

Tracking

(Blocks 1 bug)

48 Branch
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: btpp-fixlater)

Attachments

(1 attachment, 5 obsolete attachments)

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
Whiteboard: btpp-fixlater
Assignee

Comment 1

3 years ago
I'd like to work on this bug.
Assignee: nobody → ttung
Assignee

Comment 2

3 years ago
Remove the code for calling ChannelCancelRunnable / SuppressException when throw the exception.
Assignee

Comment 3

3 years ago
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.
Attachment #8809335 - Attachment is obsolete: true
Attachment #8809788 - Flags: feedback?(bkelly)
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!
Attachment #8809788 - Flags: feedback?(bkelly) → feedback+
Assignee

Comment 5

3 years ago
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.
Attachment #8809788 - Attachment is obsolete: true
Assignee

Comment 6

3 years ago
(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
Attachment #8812136 - Attachment is obsolete: true
Attachment #8814842 - Flags: review?(bkelly)
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.
Attachment #8814842 - Flags: review?(bkelly) → review+
Assignee

Comment 8

3 years ago
(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!
Assignee

Updated

3 years ago
Keywords: checkin-needed
Assignee

Comment 10

3 years ago
Sorry, I forget to modify the patch name
Keywords: checkin-needed
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 12

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/362a85a19937
Do not fail event processing if an exception is thrown in ServiceWorker. r=bkelly
Keywords: checkin-needed

Comment 13

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/362a85a19937
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee

Comment 14

2 years ago
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.
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?
Depends on: 1352101
(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.
Blocks: 1352101
No longer depends on: 1352101
You need to log in before you can comment on or make changes to this bug.