Closed Bug 1156875 Opened 9 years ago Closed 9 years ago

URL.createObjectURL leaks in JS sandbox

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: mt, Assigned: baku)

References

Details

Attachments

(3 files, 7 obsolete files)

Unless you call URL.revokeObjectURL(), URL.createObjectURL() leaks when the sandbox is destroyed.
Can you provide a small test case? This would be helpful!
Flags: needinfo?(martin.thomson)
Check out the patches on bug 1155898.  Remove the URL.revokeObjectURL() call that I added to https://dxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/fetch/test_fetch_basic.js
Flags: needinfo?(martin.thomson)
Otherwise, here's one you can run in an xpcshell test.

var sb = new Cu.Sandbox('https://example.com', { wantGlobalProperties: [ 'Blob', 'URL' ] });
Cu.evalInSandbox('var u = URL.createObjectURL(new Blob(["text"], { type: "text/plain" }));', sb);
Cu.nukeSandbox(sb);
bz, I wonder if we have a 'global' object where we can store the list of blob URIs from a sandbox.
We have a similar approach in WorkerPrivate and in nsGlobalWindow.
Flags: needinfo?(bzbarsky)
baku, any reason that you can't just use SandboxPrivate?
Thanks Martin! I'll submit a patch soon.
Flags: needinfo?(bzbarsky)
Attached patch sandbox.patch (obsolete) — Splinter Review
Attachment #8597943 - Flags: review?(bobbyholley)
Assignee: nobody → amarchesini
Could we possibly just put this on nsIGlobalObject so that it will happen more automatically?
I thought about that; it would definitely save on special-case code.  Maybe use a subordinate object on nsIGlobalObject so that the only cost to most globals is a (null) pointer and instantiate it as needed.
This patch uses nsTArray<nsCString> mHostObjectURIs.  That has the same size as a pointer until you put something in the array.
Yeah, making this automatic somehow would be great. Is somebody in this bug willing to do that?
Attachment #8597943 - Flags: review?(bobbyholley)
I'm doing it.
Attached patch global.patch (obsolete) — Splinter Review
bholley, do you want to review this patch?
Attachment #8597943 - Attachment is obsolete: true
Attachment #8598229 - Flags: review?(bobbyholley)
Comment on attachment 8598229 [details] [diff] [review]
global.patch

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

r=me modulo that I guess, though I really wish we had a way to assert that we always traversed/unlinked this stuff.

::: dom/base/nsIGlobalObject.cpp
@@ +99,5 @@
> +  if (mHostObjectURIs.IsEmpty()) {
> +    return;
> +  }
> +
> +  // Currently we only store FileImpl objects out of the main-thread and they

s/out of the/off the/

@@ +101,5 @@
> +  }
> +
> +  // Currently we only store FileImpl objects out of the main-thread and they
> +  // are not CCed.
> +  if (NS_IsMainThread()) {

Err, don't you mean _!_NS_IsMainThread() here?

IIUC, this means that the traversal basically never runs in the current patch. Please find a way to add test coverage that catches this.

::: dom/base/nsIGlobalObject.h
@@ +40,5 @@
> +
> +protected:
> +  virtual ~nsIGlobalObject();
> +
> +  nsTArray<nsCString> mHostObjectURIs;

Can we make this private?

::: dom/base/test/test_sandboxed_blob_uri.html
@@ +15,5 @@
> +    var sb = new Cu.Sandbox('https://example.com', { wantGlobalProperties: [ 'Blob', 'URL' ] });
> +    Cu.evalInSandbox('var u = URL.createObjectURL(new Blob(["text"], { type: "text/plain" }));', sb);
> +    Cu.nukeSandbox(sb);
> +
> +    ok(true, "are we leaking blobs?");

Maybe force a GC here?

::: js/xpconnect/src/Sandbox.cpp
@@ +57,5 @@
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> +
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(SandboxPrivate)
> +
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS

Nit - why the whitespace above?
Attachment #8598229 - Flags: review?(bobbyholley) → review+
Andrew, do you have any idea how we can assert that these traverse/unlink methods are always called when cycle-collecting one of the subclasses, or make it less error-prone somehow?
Flags: needinfo?(continuation)
Inheriting from multiple cycle collected classes hasn't been a very common situation so far, so we don't really have anything to help protect against that.  I'm not sure what you can do, short of static analysis.
Flags: needinfo?(continuation)
> IIUC, this means that the traversal basically never runs in the current
> patch. Please find a way to add test coverage that catches this.

Actually, it was quite orange on treeherder.
Attached patch global2.patch (obsolete) — Splinter Review
Here a second patch that unifies the WorkerPrivate with the nsIGlobalObject.
I also removed the assertions because I realized that we should not crash if the user does: URL.revokeObjectURL('blob:something_fake');
Attachment #8598545 - Flags: review?(bobbyholley)
(In reply to Bobby Holley (:bholley) from comment #15)
> Andrew, do you have any idea how we can assert that these traverse/unlink
> methods are always called when cycle-collecting one of the subclasses, or
> make it less error-prone somehow?
Hmm, should we perhaps somehow just prevent inhering from multiple CCed classes. I think so.
So in this case, if nsGlobalWindow inherits from both EventTarget and nsIGlobalObject, and we want to put some CCed stuff in nsIGlobalObject... how should that work?
oh, that way. Then just add Traverse/Unlink methods explicitly to nsIGlobalObject and call them always in child. Error prone sure. Looks like the patch does effectively that.

(I'd try to make all the globals EventTargets, and then have EventTarget>nsIGlobalObject>nsGlobalWindow class hierarchy.
Would bring some more consistency to globals.
But not in this bug.)
(In reply to Andrea Marchesini (:baku) from comment #18)
> I also removed the assertions because I realized that we should not crash if
> the user does: URL.revokeObjectURL('blob:something_fake');

Is there a test for that somewhere?  Might pay to have one.
Attached patch global2.patch (obsolete) — Splinter Review
mochitest added.
Attachment #8598545 - Attachment is obsolete: true
Attachment #8598545 - Flags: review?(bobbyholley)
Attachment #8598717 - Flags: review?(bobbyholley)
Attachment #8598717 - Flags: feedback+
Comment on attachment 8598717 [details] [diff] [review]
global2.patch

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

This whole aToBeRegistered thing seems super error-prone. Why do we need this inconsistent behavior here?

::: dom/base/nsIGlobalObject.cpp
@@ -24,5 @@
>  
>  void
>  nsIGlobalObject::RegisterHostObjectURI(const nsACString& aURI)
>  {
> -  MOZ_ASSERT(!mHostObjectURIs.Contains(aURI));

Hm, shouldn't we keep this assertion? It seems like we don't really handle duplicates here.
Attachment #8598717 - Flags: review?(bobbyholley)
> This whole aToBeRegistered thing seems super error-prone. Why do we need
> this inconsistent behavior here?

Because you can create a worker, in the worker you create a blob and a blob URL. You send the blob URL to the main-thread. Then the worker is CCed. You want to have that blob URL still alive.
This is the reason why, if we have a parent window, the blob URLs are registered in that parent nsIGlobalObject.

Otherwise, if we don't have a window, we are in a 'special worker' setup: SharedWorkers, ServiceWorkers, etc. For those, we want to do the auto-revoke when they go away. So the blob URIs are registered in the nsIGlobalObject of the worker.

Does this answer your question?
Comment on attachment 8598717 [details] [diff] [review]
global2.patch

I'm going to reintroduce the first assertion.
Attachment #8598717 - Flags: review?(bobbyholley)
Attached patch global2.patch (obsolete) — Splinter Review
Better approach without bool*.
Attachment #8598717 - Attachment is obsolete: true
Attachment #8598717 - Flags: review?(bobbyholley)
Attachment #8598821 - Flags: review?(bobbyholley)
Comment on attachment 8598821 [details] [diff] [review]
global2.patch

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

Hm, it seems kind of brittle that our policy here depends on whether or not we have a Window (what about workers-in-sandboxes etc?), but that's out-of-scope for this bug.

I do think that rather than stashing this on the runnable, we should stash something on the URL object called "mRegisteredWithMainThreadGlobal". This gets computed during the creation runnable, and never changes. All subsequent registration decisions and deregistrations decisions would be made based on that. Does that make sense?

Also the explanation in comment 26 should go into a comment somewhere here.
Attachment #8598821 - Flags: review?(bobbyholley)
> made based on that. Does that make sense?

URL.createObjectURL and removeObjectURL are static methods. We don't have any URL object around. In JS you do:

var blob = new Blob([123]);
var url = URL.createObjectURL(blob);
Flags: needinfo?(bobbyholley)
Comment on attachment 8598821 [details] [diff] [review]
global2.patch

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

Oh I see. Ok, I guess in that case just rename the methods to {Unr,R}egisteredWithMainThreadGlobal (and invert their semantics), and add the comment explaining what's going on here.
Attachment #8598821 - Flags: review+
Flags: needinfo?(bobbyholley)
Attached patch global2.patch (obsolete) — Splinter Review
This patch uses the nsIGlobalObject of the parent of the top-level worker.
This means that also for a worker created in a sandbox, we use the nsIGlobalObject of the sandbox to keep alive the blob URLs.

For SharedWorkers and ServiceWorkers only we do the registration in the WorkerPrivate.

I spoke with ehsan about if it's correct to do the registration in the WorkerPrivate also for ServiceWorkers and, we filed a spec bug, because this is unclear. In case, the changes will be implemented in a follow-up.
Attachment #8598821 - Attachment is obsolete: true
Attachment #8598988 - Flags: review?(bent.mozilla)
Attached patch global2.patch (obsolete) — Splinter Review
Attachment #8598988 - Attachment is obsolete: true
Attachment #8598988 - Flags: review?(bent.mozilla)
Attachment #8599512 - Flags: review?(bent.mozilla)
Comment on attachment 8599512 [details] [diff] [review]
global2.patch

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

r=me with these fixed:

::: dom/workers/URL.cpp
@@ +78,5 @@
>                      nsAString& aURL)
> +  : WorkerMainThreadRunnable(aWorkerPrivate)
> +  , mBlobImpl(aBlobImpl)
> +  , mURL(aURL)
> +  , mRegisteredWithMainThreadGlobal(true)

Hm, you know here whether or not aWorkerPrivate is a shared/service worker. Why wait to set this on the main thread? I think you could just get rid of this member and always do the special scope register/unregister for shared/service workers. Same for RevokeURLRunnable.

@@ +142,5 @@
> +      // Walk up to top worker object.
> +      WorkerPrivate* wp = mWorkerPrivate;
> +      while (wp->GetParent()) {
> +        wp = wp->GetParent();
> +      }

Nit:

  while (WorkerPrivate* parent = wp->GetParent()) {
    wp = parent;
  }

@@ +145,5 @@
> +        wp = wp->GetParent();
> +      }
> +
> +      nsCOMPtr<nsIScriptContext> sc = wp->GetScriptContext();
> +      if (sc) {

You need to comment here that we will leak if there's no sc, and explain the cases when that can happen.

@@ +175,5 @@
>    RevokeURLRunnable(WorkerPrivate* aWorkerPrivate,
>                      const nsAString& aURL)
> +  : WorkerMainThreadRunnable(aWorkerPrivate)
> +  , mURL(aURL)
> +  , mUnregisteredWithMainThreadGlobal(true)

You know if this is a shared/service worker here too.

@@ +203,5 @@
> +    } else {
> +      // Walk up to top worker object.
> +      WorkerPrivate* wp = mWorkerPrivate;
> +      while (wp->GetParent()) {
> +        wp = wp->GetParent();

Same nit
Attachment #8599512 - Flags: review?(bent.mozilla) → review+
Attached patch patch 1Splinter Review
Attachment #8598229 - Attachment is obsolete: true
Attached patch patch 2Splinter Review
Attachment #8599512 - Attachment is obsolete: true
Keywords: checkin-needed
Depends on: 1164199
Depends on: 1167714
Is there a reason the new member on nsIGlobalObject wasn't added next to the existing mIsDying member?  Having the data members spread out like that is a bit weird.
Flags: needinfo?(amarchesini)
(In reply to Boris Zbarsky [:bz] from comment #40)
> Is there a reason the new member on nsIGlobalObject wasn't added next to the
> existing mIsDying member?  Having the data members spread out like that is a
> bit weird.

No reasons actually. Do you want me to move it?
Flags: needinfo?(amarchesini)
Please.
Attached patch patch 3Splinter Review
Attachment #8627246 - Flags: review?(bzbarsky)
Comment on attachment 8627246 [details] [diff] [review]
patch 3

r=me
Attachment #8627246 - Flags: review?(bzbarsky) → review+
You need to log in before you can comment on or make changes to this bug.