587251 - new Worker(badURL) should throw a SECURITY_ERR

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Jeff Walden [:Waldo]]

It looks like syntax-checking only occurs at script load time when a global is created for the worker.

Comment 1

[Alexander C Rees]

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

[Olli Pettay [:smaug][[e-mail]]]

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

See nsDOMWorker::InitializeInternal

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Now in ConstructInternal at

http://mxr.mozilla.org/mozilla-central/source/dom/workers/Worker.cpp#142

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: patch

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: fixup patch to bug 744910

Comment 7

[Masatoshi Kimura [:emk]]

Attachment: patch

Rebased and folded two patches because bug 744910 has been landed.

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: patch

updated to tip

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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 :-/

Comment 10

[Masatoshi Kimura [:emk]]

Auto-revoking blob URIs will be unusable for Worker scripts until this bug is fixed.

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: patch

Rebased to tip and fixed a test depending on the current (wrong) behavior.
https://tbpl.mozilla.org/?tree=Try&rev=ea1f460b510c

Comment 12

[Masatoshi Kimura [:emk]]

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).

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 692551 [details] [diff] [review]
patch

Sorry this took so long, but Kyle is going to take this!

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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?

Comment 16

[Masatoshi Kimura [:emk]]

Attachment: new Worker(badURL) should throw a SECURITY_ERR

(In reply to Kyle Huey [:khuey] ([e-mail]) 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)" は未解決です。

Comment 17

[Masatoshi Kimura [:emk]]

Attachment: Interdiff (for ease of review)

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 19

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48578447ec42

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/48578447ec42

Comment 21

[Jean-Yves Perrier [:teoli]]

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