Last Comment Bug 587251 - new Worker(badURL) should throw a SECURITY_ERR
: new Worker(badURL) should throw a SECURITY_ERR
Status: RESOLVED FIXED
[parity-chrome][parity-ie10]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Workers (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla22
Assigned To: Masatoshi Kimura [:emk]
:
: Andrew Overholt [:overholt]
Mentors:
javascript: try { new Worker("data:ap...
Depends on: 849094
Blocks: 773132
  Show dependency treegraph
 
Reported: 2010-08-13 18:17 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2013-09-10 00:52 PDT (History)
8 users (show)
VYV03354: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (29.15 KB, patch)
2012-04-14 17:53 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
fixup patch to bug 744910 (1.98 KB, patch)
2012-04-14 21:48 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
patch (29.14 KB, patch)
2012-04-28 02:57 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
patch (29.23 KB, patch)
2012-05-20 00:29 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
patch (30.45 KB, patch)
2012-12-14 21:29 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
new Worker(badURL) should throw a SECURITY_ERR (32.74 KB, patch)
2013-03-06 14:54 PST, Masatoshi Kimura [:emk]
khuey: review+
Details | Diff | Splinter Review
Interdiff (for ease of review) (13.19 KB, patch)
2013-03-06 14:54 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2010-08-13 18:17:04 PDT
It looks like syntax-checking only occurs at script load time when a global is created for the worker.
Comment 1 Alexander C Rees 2010-08-18 04:54:15 PDT
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] 2010-08-18 05:10:00 PDT
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 (⌚ UTC+1/+2) 2011-10-14 09:44:06 PDT
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!) 2011-10-14 09:46:23 PDT
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] 2012-04-14 17:53:30 PDT
Created attachment 615096 [details] [diff] [review]
patch
Comment 6 Masatoshi Kimura [:emk] 2012-04-14 21:48:15 PDT
Created attachment 615112 [details] [diff] [review]
fixup patch to bug 744910
Comment 7 Masatoshi Kimura [:emk] 2012-04-28 02:57:23 PDT
Created attachment 619270 [details] [diff] [review]
patch

Rebased and folded two patches because bug 744910 has been landed.
Comment 8 Masatoshi Kimura [:emk] 2012-05-20 00:29:08 PDT
Created attachment 625465 [details] [diff] [review]
patch

updated to tip
Comment 9 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-06-18 11:42:01 PDT
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] 2012-07-11 19:39:04 PDT
Auto-revoking blob URIs will be unusable for Worker scripts until this bug is fixed.
Comment 11 Masatoshi Kimura [:emk] 2012-12-14 21:29:05 PST
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
Comment 12 Masatoshi Kimura [:emk] 2013-01-08 18:13:52 PST
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!) 2013-02-20 12:41:35 PST
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!) 2013-03-05 11:05:37 PST
Comment on attachment 692551 [details] [diff] [review]
patch

Sorry this took so long, but Kyle is going to take this!
Comment 15 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-03-05 16:35:28 PST
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] 2013-03-06 14:54:01 PST
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)" は未解決です。
Comment 17 Masatoshi Kimura [:emk] 2013-03-06 14:54:56 PST
Created attachment 721910 [details] [diff] [review]
Interdiff (for ease of review)
Comment 18 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-03-06 15:01:37 PST
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 20 Ryan VanderMeulen [:RyanVM] 2013-03-07 21:06:45 PST
https://hg.mozilla.org/mozilla-central/rev/48578447ec42
Comment 21 Jean-Yves Perrier [:teoli] 2013-09-10 00:52:55 PDT
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

Note You need to log in before you can comment on or make changes to this bug.