URL.createObjectURL leaks in JS sandbox

RESOLVED FIXED in Firefox 40

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mt, Assigned: baku)

Tracking

Trunk
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

3 years ago
Unless you call URL.revokeObjectURL(), URL.createObjectURL() leaks when the sandbox is destroyed.
(Assignee)

Comment 1

3 years ago
Can you provide a small test case? This would be helpful!
Flags: needinfo?(martin.thomson)
(Reporter)

Comment 2

3 years ago
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)
(Reporter)

Comment 3

3 years ago
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);
(Assignee)

Comment 4

3 years ago
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)
(Reporter)

Comment 5

3 years ago
baku, any reason that you can't just use SandboxPrivate?
(Assignee)

Comment 6

3 years ago
Thanks Martin! I'll submit a patch soon.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 7

3 years ago
Created attachment 8597943 [details] [diff] [review]
sandbox.patch
Attachment #8597943 - Flags: review?(bobbyholley)
(Assignee)

Updated

3 years ago
Assignee: nobody → amarchesini
Could we possibly just put this on nsIGlobalObject so that it will happen more automatically?
(Reporter)

Comment 9

3 years ago
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?
(Assignee)

Updated

3 years ago
Attachment #8597943 - Flags: review?(bobbyholley)
(Assignee)

Comment 12

3 years ago
I'm doing it.
(Assignee)

Comment 13

3 years ago
Created attachment 8598229 [details] [diff] [review]
global.patch

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)
(Assignee)

Comment 17

3 years ago
> 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.
(Assignee)

Comment 18

3 years ago
Created attachment 8598545 [details] [diff] [review]
global2.patch

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)
(Assignee)

Comment 19

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1307ad7b37c
(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.)
(Reporter)

Comment 23

3 years ago
(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.
(Assignee)

Comment 24

3 years ago
Created attachment 8598717 [details] [diff] [review]
global2.patch

mochitest added.
Attachment #8598545 - Attachment is obsolete: true
Attachment #8598545 - Flags: review?(bobbyholley)
Attachment #8598717 - Flags: review?(bobbyholley)
(Reporter)

Updated

3 years ago
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)
(Assignee)

Comment 26

3 years ago
> 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?
(Assignee)

Comment 27

3 years ago
Comment on attachment 8598717 [details] [diff] [review]
global2.patch

I'm going to reintroduce the first assertion.
Attachment #8598717 - Flags: review?(bobbyholley)
(Assignee)

Comment 28

3 years ago
Created attachment 8598821 [details] [diff] [review]
global2.patch

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)
(Assignee)

Comment 30

3 years ago
> 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);
(Assignee)

Updated

3 years ago
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)
(Assignee)

Comment 32

3 years ago
Created attachment 8598988 [details] [diff] [review]
global2.patch

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)
(Assignee)

Comment 33

3 years ago
Created attachment 8599512 [details] [diff] [review]
global2.patch
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+
(Assignee)

Comment 35

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=682bd72b34d1
(Assignee)

Comment 36

3 years ago
Created attachment 8602538 [details] [diff] [review]
patch 1
Attachment #8598229 - Attachment is obsolete: true
(Assignee)

Comment 37

3 years ago
Created attachment 8602539 [details] [diff] [review]
patch 2
Attachment #8599512 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 38

3 years ago
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2e84c560ac0b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/522f3549ec5b
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2e84c560ac0b
https://hg.mozilla.org/mozilla-central/rev/522f3549ec5b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1163109

Updated

3 years ago
Depends on: 1164199

Updated

3 years ago
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)
(Assignee)

Comment 41

2 years ago
(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.
(Assignee)

Comment 43

2 years ago
Created attachment 8627246 [details] [diff] [review]
patch 3
Attachment #8627246 - Flags: review?(bzbarsky)
Comment on attachment 8627246 [details] [diff] [review]
patch 3

r=me
Attachment #8627246 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 45

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd3716e4bd1
https://hg.mozilla.org/mozilla-central/rev/1fd3716e4bd1
You need to log in before you can comment on or make changes to this bug.