Closed Bug 587251 Opened 14 years ago Closed 11 years ago

new Worker(badURL) should throw a SECURITY_ERR

Categories

(Core :: DOM: Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: Waldo, Assigned: emk)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: [parity-chrome][parity-ie10])

Attachments

(2 files, 5 obsolete files)

It looks like syntax-checking only occurs at script load time when a global is created for the worker.
Whiteboard: [good first bug]
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.
The workers related files are under dom/src/threads
http://mxr.mozilla.org/mozilla-central/source/dom/src/threads/

See nsDOMWorker::InitializeInternal
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]
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #615096 - Flags: review?(bent.mozilla)
Component: DOM: Mozilla Extensions → DOM: Workers
QA Contact: general → workers
Attached patch fixup patch to bug 744910 (obsolete) — Splinter Review
Attachment #615112 - Flags: review?(bent.mozilla)
Attached patch patch (obsolete) — Splinter Review
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)
Attached patch patch (obsolete) — Splinter Review
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 :-/
Auto-revoking blob URIs will be unusable for Worker scripts until this bug is fixed.
Blocks: 773132
Attached patch patch (obsolete) — Splinter Review
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)
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).
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)
(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)
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+
Depends on: 849094
https://hg.mozilla.org/mozilla-central/rev/48578447ec42
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: