Open Bug 1254125 Opened 7 years ago Updated 5 months ago

Don't propagate errors to SharedWorker

Categories

(Core :: DOM: Workers, defect, P3)

Product:
Core
Component:
DOM: Workers
Type:
defect

Tracking

()

People

(Reporter: Ms2ger, Unassigned, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tw-dom] btpp-fixlater)

Attachments

(3 files, 2 obsolete files)

Mochitest for a SharedWorker that fails to load.
1.89 KB, patch
Details | Diff | Splinter Review
Bug 1254125 - Update web-platform-test testcase and manifest for sharedworker.
1.50 KB, patch
Details | Diff | Splinter Review
wpt
2.09 KB, patch
Details | Diff | Splinter Review 
This refers to https://html.spec.whatwg.org/multipage/workers.html#runtime-script-errors-2 and the

"For shared workers, if the error is still not handled afterwards, the error may be reported to a developer console."

Currently we follow the instructions for dedicated workers for all workers.  The error handling pathway starts at WorkerPrivate::ReportError.
Whiteboard: [tw-dom]
I would like to try on this bug.
Assignee: nobody → jdai
Whiteboard: [tw-dom] → [tw-dom] btpp-active
I sent error report to a developer console instead of broadcasting error to SharedWorkers.
After apply this patch, http://w3c-test.org/workers/semantics/run-a-worker/003.html are all passed.
There still has many shared worker testcases need to modify.

Hi khuey, May I have your early feedback on this patch?
Thank you.
Attachment #8732055 - Flags: feedback?(khuey)
Comment on attachment 8732055 [details] [diff] [review]
Bug 1254125 - Don't propagate errors to SharedWorker.

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

Looks good to me.  There should be some updates to web-platform-test manifests too, I think?  http://mxr.mozilla.org/mozilla-central/find?text=&string=run-a-worker%2F003
Attachment #8732055 - Flags: feedback?(khuey) → feedback+
Here is the summary of this patch:
- Modify test_multi_sharedWorker.html and test_sharedWorker.html.
- Modify web-platform-test metadata files to make testcases turn green.

I modified same-origin.html.ini, but it is still failure on Tryserver. Need to spend time on testing/web-platform/tests/workers/constructors/SharedWorker/same-origin.html
Shall we open another bug to trace those failure testcases?

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7556dd9f63c

Hi khuey, May I have your feedback on this patch?
Thank you.
Attachment #8733832 - Flags: feedback?(khuey)
Seems like your patch is no longer dispatching an error event when we fail to fetch the script; it seems like we should still do that.
So, the bad news is that comment 6 is correct, this patch does in fact suppress the load error because the load error goes through the same path as runtime script errors (which is what we actually want to suppress).

The good news is that we actually have a separate pathway for handling load errors that we created for ServiceWorkers in MaybeDispatchLoadFailedRunnable.  We ought to be able to hook that up with a runnable that fires ErrorEvents on our SharedWorkers.
Comment on attachment 8732055 [details] [diff] [review]
Bug 1254125 - Don't propagate errors to SharedWorker.

Per comment 8.
Attachment #8732055 - Flags: feedback+ → feedback-
(A very easy way to test this is to make the Worker in test_404.html a SharedWorker, and use worker.port for message handling so the test doesn't throw an exception.)
I only caught this because workers/constructors/SharedWorker/same-origin.html checks it, fwiw.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #11)
> Created attachment 8733996 [details] [diff] [review]
> Mochitest for a SharedWorker that fails to load.

Thank you provide me Mochitest testcase, but this testcase is a opposite condition of http://w3c-test.org/workers/semantics/run-a-worker/003.html[1] which means I only can pass either one of test. If I follow comment 8, I can't pass 003.html[1] testcase. 


[1] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/workers/semantics/run-a-worker/003.html
Flags: needinfo?(khuey)
Per comment 14.
Flags: needinfo?(khuey)
Per comment 14, modified workers/semantics/run-a-worker/003.html.
Attachment #8732055 - Attachment is obsolete: true
Attachment #8733832 - Attachment is obsolete: true
Hi Kyle,

1. From comment 11 and comment 14, those testcases can pass even no modify any code.
I am not sure should I need to continue doing comment 8 in this bug? 

2. I don't know how to generate a runtime error hit |WorkerPrivate::ReportError|. I follow this website[1], but with no luck. 

Please correct me if I misunderstand anything, thanks!

[1]http://www.htmlgoodies.com/primers/jsp/article.php/3610081/Javascript-Basics-Part-11.htm
Flags: needinfo?(khuey)
Attached patch wpt — — Splinter Review 
This test captures what the spec says for both load and runtime errors.  You'll see that we don't pass it before or after the patch.  But we should check and see if Chrome passes the test or if the spec is just wrong.
Flags: needinfo?(khuey)
(In reply to John Dai[:johnz][:jdai] from comment #17)
> Hi Kyle,
> 
> 1. From comment 11 and comment 14, those testcases can pass even no modify
> any code.
> I am not sure should I need to continue doing comment 8 in this bug? 

See the previous testcase. But also check what Chrome does! See if it matches the spec or us.

> 2. I don't know how to generate a runtime error hit
> |WorkerPrivate::ReportError|. I follow this website[1], but with no luck. 

Any sort of javascript exception should be enough.  I think the test I posted above should trigger that pathway.
John has several html bugs on his plate. He will be back to this bug afterwards.
Whiteboard: [tw-dom] btpp-active → [tw-dom] btpp-fixlater
Assignee: jdai → nobody
Priority: -- → P3

Note that we're now discussing spec changes in https://github.com/whatwg/html/issues/5323

See Also: → 1626143
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.