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

RESOLVED FIXED in mozilla24

Status

()

Core
DOM: Workers
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Yoric, Assigned: baku)

Tracking

(Blocks: 1 bug, {crash})

unspecified
mozilla24
crash
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment, 4 obsolete attachments)

12.14 KB, patch
Details | Diff | Splinter Review
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.

Updated

5 years ago
Crash Signature: [@ nsCOMPtr<nsPIDOMWindow>::operator->() | CreateURLRunnable::MainThreadRun()]
Keywords: crash
Created attachment 763496 [details] [diff] [review]
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.
(Assignee)

Comment 2

5 years ago
Created attachment 764255 [details] [diff] [review]
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.
Attachment #763496 - Attachment is obsolete: true
Attachment #764255 - Flags: feedback?(bent.mozilla)
Comment on attachment 764255 [details] [diff] [review]
WIP

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

This looks like it's on the right track!
Attachment #764255 - Flags: feedback?(bent.mozilla) → feedback+
(Assignee)

Comment 4

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

This leaks, but not for this patch. I'm going to file a bug for the leaking in JSM + worker.
Attachment #764255 - Attachment is obsolete: true
Attachment #764656 - Flags: review?(bent.mozilla)
(Assignee)

Updated

5 years ago
Depends on: 884738
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/
Attachment #764656 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 6

5 years ago
Created attachment 765488 [details] [diff] [review]
patch
Attachment #764656 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 10

5 years ago
Created attachment 766365 [details] [diff] [review]
patch
Attachment #765488 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e762fad3e026
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24

Updated

5 years ago
Depends on: 890928
You need to log in before you can comment on or make changes to this bug.