813253 - URL.createobjectURL in WebWorker

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[key.robrowser]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.20 (KHTML, like Gecko) Chrome/25.0.1329.0 Safari/537.20

Steps to reproduce:

I try to use URL.createObjectURL in a WebWorker to transfer a lot of images content (parsed from a big data) into the main thread without using postMessage (postMessage cause a little freeze because sending too much data).


Actual results:

The object "URL" isn't defined in WebWorker context.


Expected results:

From the reference, URL should be exposed to window AND WebWorker context

"Blob URIs are created and revoked using methods exposed on the URL object, supported by global objects Window [HTML] and WorkerGlobalScope [Web Workers]"

http://www.w3.org/TR/FileAPI/#creating-revoking

Comment 1

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

Andrea, does this interest you at all?

Comment 2

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 3

[Andrea Marchesini [:baku]]

Attachment: patch

better mochitests.

Comment 4

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

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

This looks like it's on the right track but can you use the WebIDL generator for this JSClass stuff? I don't want to add any more manual JSAPI if we can avoid it.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch b

Comment 6

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

Comment on attachment 686105 [details] [diff] [review]
patch b

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

::: dom/bindings/Bindings.conf
@@ +276,5 @@
>  {
>      'workers': True,
>  }],
>  
> +'MediaStream': [{

This is unrelated, right?

::: dom/workers/URL.cpp
@@ +8,5 @@
> +
> +#include "jsapi.h"
> +#include "jsfriendapi.h"
> +#include "nsJSUtils.h"
> +#include "nsStringGlue.h"

Hm, there are several headers in this file that you definitely do not need.

@@ +21,5 @@
> +#include "nsHostObjectProtocolHandler.h"
> +
> +#include "nsIDOMFile.h"
> +
> +#include "mozilla/dom/URLBinding.h"

Nit: You already included this in the header.

@@ +23,5 @@
> +#include "nsIDOMFile.h"
> +
> +#include "mozilla/dom/URLBinding.h"
> +
> +BEGIN_WORKERS_NAMESPACE

Just USING_WORKERS_NAMESPACE here.

@@ +84,5 @@
> +    mWorkerPrivate->AssertIsOnWorkerThread();
> +    mSyncQueueKey = mWorkerPrivate->CreateNewSyncLoop();
> +
> +    if (NS_FAILED(NS_DispatchToMainThread(this, NS_DISPATCH_NORMAL))) {
> +      JS_ReportError(aCx, "Failed to dispatch to main thread!");

You need to destroy the sync loop here if the dispatch fails.

@@ +113,5 @@
> +// This class creates an URL from a DOM Blob on the main thread.
> +class CreateURLRunnable : public URLRunnable
> +{
> +protected:
> +  WorkerPrivate* mWorkerPrivate;

You don't need another worker member, the base class has one already.

@@ +116,5 @@
> +protected:
> +  WorkerPrivate* mWorkerPrivate;
> +  nsIDOMBlob* mBlob;
> +  const mozilla::dom::objectURLOptionsWorkers& mOptions;
> +  nsString& mURL;

This works but it's dangerous to mutate an object that lives on another thread's stack. Let's just make this a real nsString and then add a getter for it so that the worker thread can get it after the call completes.

@@ +135,5 @@
> +  {
> +    AssertIsOnMainThread();
> +
> +    nsCOMPtr<nsPIDOMWindow> w = mWorkerPrivate->GetWindow();
> +    nsGlobalWindow* window = static_cast<nsGlobalWindow*>(w.get());

GetExtantDoc lives on nsPIDOMWindow, no need to cast to nsGlobalWindow here.

@@ +141,5 @@
> +    if (!window || !window->GetExtantDoc()) {
> +      return;
> +    }
> +
> +    nsIDocument* doc = window->GetExtantDoc();

No need to call this twice. Call it once and then null-check the result instead.

@@ +147,5 @@
> +    nsCString url;
> +    nsresult rv = nsHostObjectProtocolHandler::AddDataEntry(NS_LITERAL_CSTRING(BLOBURI_SCHEME),
> +      mBlob, doc->NodePrincipal(), url);
> +    if (NS_FAILED(rv)) {
> +      return;

Should probably issue a warning here.

@@ +159,5 @@
> +// This class revokes an URL on the main thread.
> +class RevokeURLRunnable : public URLRunnable
> +{
> +protected:
> +  WorkerPrivate* mWorkerPrivate;

Unneeded.

@@ +160,5 @@
> +class RevokeURLRunnable : public URLRunnable
> +{
> +protected:
> +  WorkerPrivate* mWorkerPrivate;
> +  const nsAString& mURL;

Same here. Just make a copy.

@@ +183,5 @@
> +    }
> +
> +    NS_LossyConvertUTF16toASCII asciiurl(mURL);
> +
> +    nsIPrincipal* winPrincipal = window->GetPrincipal();

Hm, why are you using the window principal instead of the document principal like in CreateURLRunnable?

@@ +203,5 @@
> +    }
> +  }
> +};
> +
> +JSObject*

Nit: Add a |// static| line above, here and for the other static methods.

::: dom/workers/URL.h
@@ +8,5 @@
> +#define mozilla_dom_workers_url_h__
> +
> +#include "mozilla/dom/BindingUtils.h"
> +#include "mozilla/dom/TypedArray.h"
> +#include "mozilla/dom/URLBinding.h"

You shouldn't need anything but URLBinding.h in this block. The other stuff should move to the cpp.

@@ +11,5 @@
> +#include "mozilla/dom/TypedArray.h"
> +#include "mozilla/dom/URLBinding.h"
> +
> +#include "EventTarget.h"
> +#include "Workers.h"

Nit: This isn't needed, EventTarget will include it.

@@ +15,5 @@
> +#include "Workers.h"
> +
> +BEGIN_WORKERS_NAMESPACE
> +
> +class URL : public EventTarget {

Nit: { on next line.

@@ +28,5 @@
> +                  const nsAString& aUrl);
> +
> +public: // Initialization and creation
> +  static JSObject*
> +  Create(JSContext* aCx, JSObject *aGlobal);

This shouldn't be needed, see comment in WorkerScope.cpp.

::: dom/workers/WorkerScope.cpp
@@ +402,5 @@
>      return true;
>    }
>  
>    static JSBool
> +  GetURL(JSContext* aCx, JSHandleObject aObj, JSHandleId aIdval, JSMutableHandleValue aVp)

None of the changes in this file should be needed... You should just be able to just call URLBinding_workers::GetConstructorObject in the place where we initialize the rest of the paris bindings.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to ben turner [:bent] from comment #6)
> Comment on attachment 686105 [details] [diff] [review]
> patch b
> 
> Review of attachment 686105 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bindings/Bindings.conf
> @@ +276,5 @@
> >  {
> >      'workers': True,
> >  }],
> >  
> > +'MediaStream': [{
> 
> This is unrelated, right?

No, this patch needs a definition of MediaStream in workers.

Comment 8

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

(In reply to :Ms2ger from comment #7)
> No, this patch needs a definition of MediaStream in workers.

I don't understand why... And I don't see where we hook that into the global in this patch either...

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to ben turner [:bent] from comment #8)
> (In reply to :Ms2ger from comment #7)
> > No, this patch needs a definition of MediaStream in workers.
> 
> I don't understand why... And I don't see where we hook that into the global
> in this patch either...

interface URL {
...
  static DOMString? createObjectURL(MediaStream stream, optional objectURLOptions options);
...
};

Note that we unwrap to JSObject there, so there is no actual implementation of MediaStream in workers. This is exactly what we do with all the other argument types that don't exist in workers, like xhr.send(Document).

Comment 10

[Andrea Marchesini [:baku]]

Attachment: patch c

Now that bug 816088 is landed, this patch can be reviewed again.

Comment 11

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

Comment on attachment 707559 [details] [diff] [review]
patch c

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

This looks pretty good but we need error reporting. Some nits too.

::: dom/workers/URL.cpp
@@ +64,5 @@
> +    }
> +  };
> +
> +public:
> +  URLRunnable(WorkerPrivate* aWorkerPrivate)

This should be protected (even though the abstract method makes this unconstructable)

@@ +78,5 @@
> +    mSyncQueueKey = mWorkerPrivate->CreateNewSyncLoop();
> +
> +    if (NS_FAILED(NS_DispatchToMainThread(this, NS_DISPATCH_NORMAL))) {
> +      JS_ReportError(aCx, "Failed to dispatch to main thread!");
> +      mWorkerPrivate->StopSyncLoop(mSyncQueueKey, true);

Nit: That last arg should be false. It won't make any difference in this case but it's more correct.

@@ +86,5 @@
> +    return mWorkerPrivate->RunSyncLoop(aCx, mSyncQueueKey);
> +  }
> +
> +  virtual void
> +  MainThreadRun() = 0;

Hm, if this is void you won't be able to report errors to the worker. I think this should be changed so that we can report a DOM exception. Also, it should be protected.

@@ +88,5 @@
> +
> +  virtual void
> +  MainThreadRun() = 0;
> +
> +  NS_IMETHOD Run()

This should be private since you don't want subclasses to call this.

@@ +106,5 @@
> +
> +// This class creates an URL from a DOM Blob on the main thread.
> +class CreateURLRunnable : public URLRunnable
> +{
> +protected:

This should be private, you're not sharing with anything.

@@ +134,5 @@
> +    }
> +
> +
> +    nsCString url;
> +    nsresult rv = nsHostObjectProtocolHandler::AddDataEntry(NS_LITERAL_CSTRING(BLOBURI_SCHEME),

Hm, is this past 80 chars? Rewrap if so.

@@ +135,5 @@
> +
> +
> +    nsCString url;
> +    nsresult rv = nsHostObjectProtocolHandler::AddDataEntry(NS_LITERAL_CSTRING(BLOBURI_SCHEME),
> +      mBlob, doc->NodePrincipal(), url);

Nit: line these up with the (

@@ +136,5 @@
> +
> +    nsCString url;
> +    nsresult rv = nsHostObjectProtocolHandler::AddDataEntry(NS_LITERAL_CSTRING(BLOBURI_SCHEME),
> +      mBlob, doc->NodePrincipal(), url);
> +    if (NS_FAILED(rv)) {

Yeah, we really want to be able to report this.

@@ +149,5 @@
> +
> +// This class revokes an URL on the main thread.
> +class RevokeURLRunnable : public URLRunnable
> +{
> +protected:

Private.

@@ +170,5 @@
> +    if (!doc) {
> +      return;
> +    }
> +
> +    NS_LossyConvertUTF16toASCII asciiurl(mURL);

Is this guaranteed to be ASCII? You used NS_ConvertUTF8toUTF16 above, so either use UTF8 here or change the other to ASCII too.

@@ +188,5 @@
> +
> +// static
> +void
> +URL::CreateObjectURL(const WorkerGlobalObject& aGlobal,
> +                     JSObject* aArgc, const objectURLOptionsWorkers& aOptions,

"aArgc" is what we usually use for "argument count", how about you rename this "aBlob"?

@@ +203,5 @@
> +  }
> +
> +  nsRefPtr<CreateURLRunnable> runnable =
> +    new CreateURLRunnable(workerPrivate, blob, aOptions, aResult);
> +  runnable->Dispatch(aGlobal.GetContext());

Here and below you should do:

  if (!runnable->Dispatch(...)) {
    JS_ReportPendingException(cx);
  }

And since you are going to be using the JSContext three times it's better to make a stack var to hold it rather than call aGlobal.GetContext() each time.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: patch d

Comment 13

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

Comment on attachment 709664 [details] [diff] [review]
patch d

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

r=me with these fixed. Thanks!

::: dom/workers/URL.cpp
@@ +18,5 @@
> +#include "nsIDOMFile.h"
> +
> +USING_WORKERS_NAMESPACE
> +
> +using mozilla::dom::workers::exceptions::ThrowDOMExceptionForNSResult;

You're not actually using this.

@@ +123,5 @@
> +  : URLRunnable(aWorkerPrivate),
> +    mBlob(aBlob),
> +    mOptions(aOptions),
> +    mURL(aURL)
> +  {}

Let's assert aBlob here.

@@ +137,5 @@
> +      SetDOMStringToNull(mURL);
> +      return;
> +    }
> +
> +

Nit: Extra line here.

@@ +202,5 @@
> +{
> +  JSContext* cx = aGlobal.GetContext();
> +  WorkerPrivate* workerPrivate = GetWorkerPrivateFromContext(cx);
> +
> +  aResult.Truncate();

This shouldn't be necessary now, you'll always set to null or a real string.

@@ +207,5 @@
> +
> +  nsCOMPtr<nsIDOMBlob> blob = file::GetDOMBlobFromJSObject(aBlob);
> +  if (!blob) {
> +    SetDOMStringToNull(aResult);
> +    aRv.Throw(NS_ERROR_INVALID_ARG);

You shouldn't throw here, right?

@@ +228,5 @@
> +  WorkerPrivate* workerPrivate = GetWorkerPrivateFromContext(cx);
> +
> +  nsRefPtr<RevokeURLRunnable> runnable =
> +    new RevokeURLRunnable(workerPrivate, aUrl);
> +  runnable->Dispatch(cx);

You need the same error reporting here as in CreateObjectURL.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to ben turner [:bent] from comment #13)
> @@ +207,5 @@
> > +
> > +  nsCOMPtr<nsIDOMBlob> blob = file::GetDOMBlobFromJSObject(aBlob);
> > +  if (!blob) {
> > +    SetDOMStringToNull(aResult);
> > +    aRv.Throw(NS_ERROR_INVALID_ARG);
> 
> You shouldn't throw here, right?

Why shouldn't this throw? This is hit if you pass null or a non-blob to the function, afaict.

Comment 16

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

(In reply to :Ms2ger from comment #15)
> Why shouldn't this throw? This is hit if you pass null or a non-blob to the
> function, afaict.

Because the spec doesn't say anything about throwing exceptions... It only says that failure results in a null return as far as I know.

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to ben turner [:bent] from comment #16)
> (In reply to :Ms2ger from comment #15)
> > Why shouldn't this throw? This is hit if you pass null or a non-blob to the
> > function, afaict.
> 
> Because the spec doesn't say anything about throwing exceptions... It only
> says that failure results in a null return as far as I know.

The URL spec doesn't need to talk about it; this failure is handled by WebIDL. The only reason that the bindings don't throw an exception before we get here is the weird setup for Blobs in workers.

Comment 18

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

Ah, so this needs to throw TypeError?

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Strictly speaking, yes.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: patch

Is this better? I sent it to try server.

Comment 21

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

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

Looks great!

Comment 22

[Masatoshi Kimura [:emk]]

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

>+  if (!blob) {
>+    SetDOMStringToNull(aResult);
>+    aRv.Throw(NS_ERROR_TYPE_ERR);
>+    return;
>+  }

Do you have a test about this? I'm surprised tests do not fail assertions by this change.
https://mxr.mozilla.org/mozilla-central/source/dom/bindings/ErrorResult.h#45

Comment 23

[Andrea Marchesini [:baku]]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=86550fed3486 almost green on try.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f82f74de11f

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7f82f74de11f

Comment 27

[Boris Zbarsky [:bzbarsky]]

Doesn't this need to be marked "concrete: False" in workers too?

Comment 28

[Tom S [:evilpie]]

Mentioned on  https://developer.mozilla.org/en-US/docs/DOM/Worker/Functions_available_to_workers and https://developer.mozilla.org/en-US/docs/Firefox_21_for_developers.