883784 - [Workers] Calling URL.createObjectURL from a worker created by a jsm causes crash by nsCOMPtr

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Relevant parts of the stack:
 0:07.81 ###!!! ABORT: You can't dereference a NULL nsCOMPtr with operator->().: 'mRawPtr != 0', file ../../../dist/include/nsCOMPtr.h, line 839
 0:07.81 nsCOMPtr<nsPIDOMWindow>::operator->() const+0x0000004A [/Users/david/Documents/Code/mc-osfile/obj-x86_64-apple-darwin11.4.2/dist/NightlyDebug.app/Contents/MacOS/XUL +0x0061012A]
 0:07.81 CreateURLRunnable::MainThreadRun()+0x00000065 [/Users/david/Documents/Code/mc-osfile/obj-x86_64-apple-darwin11.4.2/dist/NightlyDebug.app/Contents/MacOS/XUL +0x015E7245]
 0:07.81 URLRunnable::Run()+0x00000023 [/Users/david/Documents/Code/mc-osfile/obj-x86_64-apple-darwin11.4.2/dist/NightlyDebug.app/Contents/MacOS/XUL +0x015E6A33]
 0:07.82 nsThread::ProcessNextEvent(bool, bool*)+0x00000676 [/Users/david/Documents/Code/mc-osfile/obj-x86_64-apple-darwin11.4.2/dist/NightlyDebug.app/Contents/MacOS/XUL +0x02D83166]

I'll try and provide a reproducible test.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Reproducing the crash

Here's a sample that reproduces the crash. Same worker loaded directly without going through a .jsm works. The offending JS code is here:
http://dxr.mozilla.org/mozilla-central/source/toolkit/components/workerloader/require.js?from=require.js#l197

@Baku, tell me if you want me to minimize further.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: WIP

I included your test. Thanks a lot for it!
This is a work-in-progress. It works but it leaks... I want to receive a feedback from bent before continuing debugging/fixing the leak.

Comment 3

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

Comment on attachment 764255 [details] [diff] [review]
WIP

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

This looks like it's on the right track!

Comment 4

[Andrea Marchesini [:baku]]

Attachment: patch

This leaks, but not for this patch. I'm going to file a bug for the leaking in JSM + worker.

Comment 5

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

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

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

Looks good! r=me with these addressed:

::: dom/workers/URL.cpp
@@ +137,2 @@
>      nsCOMPtr<nsPIDOMWindow> window = mWorkerPrivate->GetWindow();
> +    if (!window) {

Nit: Most times you have code for both conditions (|foo| and |!foo|) it's best to have the |foo| clause first, otherwise you have to reason about "else !foo"... Here and in the RevokeRunnable.

@@ +137,3 @@
>      nsCOMPtr<nsPIDOMWindow> window = mWorkerPrivate->GetWindow();
> +    if (!window) {
> +      if (mWorkerPrivate->IsChromeWorker()) {

I think you should assert this instead, here and in RevokeRunnable.

@@ +216,5 @@
>        nsHostObjectProtocolHandler::GetDataEntryPrincipal(url);
>  
>      bool subsumes;
> +    if (urlPrincipal &&
> +        NS_SUCCEEDED(principal->Subsumes(urlPrincipal, &subsumes)) &&

You don't need to worry about this in the |!window| case, you should always call |mWorkerPrivate->UnregisterHostObjectUri()|.

Basically the chrome worker principal will always subsume everything anyway.

::: dom/workers/WorkerPrivate.cpp
@@ +529,5 @@
>  class MainThreadReleaseRunnable : public nsRunnable
>  {
>    nsCOMPtr<nsIThread> mThread;
>    nsTArray<nsCOMPtr<nsISupports> > mDoomed;
> +  nsTArray<nsCString> mUris;

Nit: mHostObjectURIs, and aHostObjectURIs below

@@ +4312,5 @@
>  #endif
>  
> +template <class Derived>
> +void
> +WorkerPrivateParent<Derived>::RegisterHostObjectUri(const nsACString& aUri)

AssertIsOnMainThread in all these methods

::: dom/workers/WorkerPrivate.h
@@ +276,5 @@
>  
>    // Only used for top level workers.
>    nsTArray<nsRefPtr<WorkerRunnable> > mQueuedRunnables;
>  
> +  // Only for ChromeWorkers without window.

Also say that it's only touched on the main thread.

@@ +618,5 @@
>    AssertInnerWindowIsCorrect() const
>    { }
>  #endif
> +
> +  void RegisterHostObjectUri(const nsACString& aUri);

Nit: Here and everywhere s/Uri/URI/

Comment 6

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d3827f914c

Comment 8

[Ryan VanderMeulen [:RyanVM]]

http://mozillamemes.tumblr.com/post/19498220636/try-server-takes-the-beatings-so-mozilla-inbound

Backed out for mochitest assertions.
https://hg.mozilla.org/integration/mozilla-inbound/rev/094d0baab7a7

https://tbpl.mozilla.org/php/getParsedLog.php?id=24399348&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=24400280&tree=Mozilla-Inbound

Comment 9

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=ab7aa729fed0

Comment 10

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e762fad3e026

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e762fad3e026

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e09f1f4c0630