new Worker(badURL) should throw a SECURITY_ERR

RESOLVED FIXED in mozilla22

Status

()

Core
DOM: Workers
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Waldo, Assigned: emk)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla22
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [parity-chrome][parity-ie10], URL)

Attachments

(2 attachments, 5 obsolete attachments)

It looks like syntax-checking only occurs at script load time when a global is created for the worker.
Whiteboard: [good first bug]

Comment 1

7 years ago
Hi,

I'd like to tackle this bug, however, as it is my first look inside the mozilla code base, some assistance on which file(s) I should be looking at would be very helpful.

Comment 2

7 years ago
The workers related files are under dom/src/threads
http://mxr.mozilla.org/mozilla-central/source/dom/src/threads/

See nsDOMWorker::InitializeInternal
Now in ConstructInternal at

http://mxr.mozilla.org/mozilla-central/source/dom/workers/Worker.cpp#142
The problem is that 'new Worker(badURL)' can be called on any thread, and our URI resolution code is main-thread-only. We can't tell it's a bad URL until we get back to the main thread, so throwing an exception is not possible.
Whiteboard: [good first bug]
(Assignee)

Comment 5

5 years ago
Created attachment 615096 [details] [diff] [review]
patch
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #615096 - Flags: review?(bent.mozilla)
(Assignee)

Updated

5 years ago
Component: DOM: Mozilla Extensions → DOM: Workers
QA Contact: general → workers
(Assignee)

Comment 6

5 years ago
Created attachment 615112 [details] [diff] [review]
fixup patch to bug 744910
Attachment #615112 - Flags: review?(bent.mozilla)
(Assignee)

Comment 7

5 years ago
Created attachment 619270 [details] [diff] [review]
patch

Rebased and folded two patches because bug 744910 has been landed.
Attachment #615096 - Attachment is obsolete: true
Attachment #615112 - Attachment is obsolete: true
Attachment #619270 - Flags: review?(bent.mozilla)
Attachment #615096 - Flags: review?(bent.mozilla)
Attachment #615112 - Flags: review?(bent.mozilla)
(Assignee)

Comment 8

5 years ago
Created attachment 625465 [details] [diff] [review]
patch

updated to tip
Attachment #619270 - Attachment is obsolete: true
Attachment #625465 - Flags: review?(bent.mozilla)
Attachment #619270 - Flags: review?(bent.mozilla)
Hi Masatoshi,

Sorry I haven't gotten to review this yet, we have some pretty time-critical stuff going on that I'm swamped with. I'll try to get to at the beginning of July :-/

Updated

5 years ago
Keywords: dev-doc-needed
(Assignee)

Comment 10

5 years ago
Auto-revoking blob URIs will be unusable for Worker scripts until this bug is fixed.
Blocks: 773132
(Assignee)

Comment 11

5 years ago
Created attachment 692551 [details] [diff] [review]
patch

Rebased to tip and fixed a test depending on the current (wrong) behavior.
https://tbpl.mozilla.org/?tree=Try&rev=ea1f460b510c
Attachment #625465 - Attachment is obsolete: true
Attachment #625465 - Flags: review?(bent.mozilla)
Attachment #692551 - Flags: review?(bent.mozilla)
(Assignee)

Comment 12

4 years ago
Ping?
It's really annoying I have to set a window.onerror handler to catch errors from |new Worker()|. Furthermore, only for Firefox (Chrome and IE10 throws as required by the spec).
(Assignee)

Updated

4 years ago
Whiteboard: [parity-chrome][parity-ie10]
I'll try to get to this as soon as we get b2g out the door (i'm told that's going to be rather soon, next week or so?)
Comment on attachment 692551 [details] [diff] [review]
patch

Sorry this took so long, but Kyle is going to take this!
Attachment #692551 - Flags: review?(bent.mozilla) → review?(khuey)
Comment on attachment 692551 [details] [diff] [review]
patch

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

Pretty straightforward comments, but I would like to see it again before I r+.

::: dom/workers/ScriptLoader.cpp
@@ +524,5 @@
>  
>  NS_IMPL_THREADSAFE_ISUPPORTS2(ScriptLoaderRunnable, nsIRunnable,
>                                                      nsIStreamLoaderObserver)
>  
> +class StopSyncLoopRunnable MOZ_FINAL : public WorkerSyncRunnable

I would prefer if you moved MainThreadSyncRunnable out of XMLHttpRequest.cpp and into WorkerPrivate.h, and then inherit from that.

@@ +528,5 @@
> +class StopSyncLoopRunnable MOZ_FINAL : public WorkerSyncRunnable
> +{
> +public:
> +  StopSyncLoopRunnable(WorkerPrivate* aWorkerPrivate,
> +                        uint32_t aSyncQueueKey)

Nit: fix indent.

@@ +560,5 @@
> +  PostRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate, bool aRunResult)
> +   MOZ_OVERRIDE
> +  {
> +    aWorkerPrivate->AssertIsOnWorkerThread();
> +    aWorkerPrivate->StopSyncLoop(mSyncQueueKey, true);

Why is this in PostRun instead of WorkerRun?

@@ +564,5 @@
> +    aWorkerPrivate->StopSyncLoop(mSyncQueueKey, true);
> +  }
> +};
> +
> +class ChannelGetterRunnable MOZ_FINAL : public nsIRunnable

Nit: inherit from nsRunnable.

@@ +573,5 @@
> +  nsIChannel** mChannel;
> +  nsresult mResult;
> +
> +public:
> +  NS_DECL_ISUPPORTS

And then use NS_DECL_ISUPPORTS_INHERITED.

@@ +575,5 @@
> +
> +public:
> +  NS_DECL_ISUPPORTS
> +
> +  ChannelGetterRunnable(WorkerPrivate* aWorkerPrivate,

Nit: aParentWorker

@@ +627,5 @@
> +  }
> +
> +};
> +
> +NS_IMPL_THREADSAFE_ISUPPORTS1(ChannelGetterRunnable, nsIRunnable)

And then you don't need this anymore.

@@ +868,5 @@
> +                     nsIChannel** aChannel)
> +{
> +  aParent->AssertIsOnWorkerThread();
> +
> +  uint32_t syncQueueKey = aParent->CreateNewSyncLoop();

Please use AutoSyncLoop holder here.  It shouldn't change anything since NS_DispatchToMainThread shouldn't fail, but if someone adds code in the future it might stop them from ending up with a sync loop that never quits.

@@ +888,5 @@
> +
> +void ReportLoadError(JSContext* aCx, const nsString& aURL,
> +                     nsresult aLoadResult, bool aIsMainThread)
> +{
> +  NS_ConvertUTF16toUTF8 url(aURL);

JS_ReportError can't handle UTF-8.

::: dom/workers/ScriptLoader.h
@@ +24,5 @@
> +ChannelFromScriptURL(nsIPrincipal* aPrincipal,
> +                     nsIURI* aBaseURI,
> +                     nsIDocument* aParentDoc,
> +                     const nsString& aScriptURL,
> +                     nsIChannel** aChannel);

Lets name these something that makes it clear what thread they belong on.  ChannelFromScriptURLMainThread and ChannelFromScriptURLWorkerThread, perhaps?
Attachment #692551 - Flags: review?(khuey)
(Assignee)

Comment 16

4 years ago
Created attachment 721908 [details] [diff] [review]
new Worker(badURL) should throw a SECURITY_ERR

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #15)
> @@ +573,5 @@
> > +  nsIChannel** mChannel;
> > +  nsresult mResult;
> > +
> > +public:
> > +  NS_DECL_ISUPPORTS
> 
> And then use NS_DECL_ISUPPORTS_INHERITED.

I removed NS_DECL_ISUPPORTS completely because otherwise link failed with following errors:
108:08.60 ScriptLoader.obj : error LNK2001: 外部シンボル ""public: virtual enum
tag_nsresult __stdcall `anonymous namespace'::ChannelGetterRunnable::QueryInterf
ace(struct nsID const &,void * *)" (?QueryInterface@ChannelGetterRunnable@?A0x20
0081d4@@UAG?AW4tag_nsresult@@ABUnsID@@PAPAX@Z)" は未解決です。
108:08.60
108:08.62 ScriptLoader.obj : error LNK2001: 外部シンボル ""public: virtual unsig
ned long __stdcall `anonymous namespace'::ChannelGetterRunnable::AddRef(void)" (
?AddRef@ChannelGetterRunnable@?A0x200081d4@@UAGKXZ)" は未解決です。
108:08.62
108:08.62 ScriptLoader.obj : error LNK2001: 外部シンボル ""public: virtual unsig
ned long __stdcall `anonymous namespace'::ChannelGetterRunnable::Release(void)"
(?Release@ChannelGetterRunnable@?A0x200081d4@@UAGKXZ)" は未解決です。
Attachment #692551 - Attachment is obsolete: true
Attachment #721908 - Flags: review?(khuey)
(Assignee)

Comment 17

4 years ago
Created attachment 721910 [details] [diff] [review]
Interdiff (for ease of review)
Comment on attachment 721908 [details] [diff] [review]
new Worker(badURL) should throw a SECURITY_ERR

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

JS_ReportError doesn't handle UTF-16 either.  I think you need to use NS_LossyConvertUTF16ToASCII here.

r=me with that.
Attachment #721908 - Flags: review?(khuey) → review+
(Assignee)

Comment 19

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/48578447ec42
Flags: in-testsuite+
Depends on: 849094
https://hg.mozilla.org/mozilla-central/rev/48578447ec42
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Updated: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/22
Added a note about when a SECURITY_ERR is thrown to the ctor documentation.
https://developer.mozilla.org/en-US/docs/Web/API/Worker.Worker
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.