URL.createobjectURL in WebWorker

RESOLVED FIXED in mozilla21

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: key.robrowser, Assigned: baku)

Tracking

({dev-doc-complete})

Trunk
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

Reporter

Description

7 years ago
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
Component: Untriaged → DOM: Workers
Product: Firefox → Core
Andrea, does this interest you at all?
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Version: 16 Branch → Trunk
Assignee

Updated

7 years ago
Assignee: nobody → amarchesini
Blocks: 668680
Assignee

Comment 2

7 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #684689 - Flags: review?(bent.mozilla)
Assignee

Comment 3

7 years ago
Posted patch patch (obsolete) — Splinter Review
better mochitests.
Attachment #684689 - Attachment is obsolete: true
Attachment #684689 - Flags: review?(bent.mozilla)
Attachment #684691 - Flags: review?(bent.mozilla)
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.
Assignee

Updated

7 years ago
Depends on: 816088
Assignee

Comment 5

7 years ago
Posted patch patch b (obsolete) — Splinter Review
Attachment #684691 - Attachment is obsolete: true
Attachment #684691 - Flags: review?(bent.mozilla)
Attachment #686105 - Flags: feedback?(bent.mozilla)
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.
Attachment #686105 - Flags: feedback?(bent.mozilla)
(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.
(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...
(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).
Assignee

Comment 10

7 years ago
Posted patch patch c (obsolete) — Splinter Review
Now that bug 816088 is landed, this patch can be reviewed again.
Attachment #686105 - Attachment is obsolete: true
Attachment #707559 - Flags: review?(bent.mozilla)
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.
Attachment #707559 - Flags: review?(bent.mozilla)
Assignee

Comment 12

6 years ago
Posted patch patch d (obsolete) — Splinter Review
Attachment #707559 - Attachment is obsolete: true
Attachment #709664 - Flags: review?(bent.mozilla)
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.
Attachment #709664 - Flags: review?(bent.mozilla) → review+
Assignee

Comment 14

6 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #709664 - Attachment is obsolete: true
Attachment #710168 - Flags: review+
(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.
(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.
(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.
Ah, so this needs to throw TypeError?
Strictly speaking, yes.
Assignee

Comment 20

6 years ago
Posted patch patch (obsolete) — Splinter Review
Is this better? I sent it to try server.
Attachment #710168 - Attachment is obsolete: true
Attachment #710222 - Flags: review+
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
Assignee

Comment 23

6 years ago
Posted patch patchSplinter Review
https://tbpl.mozilla.org/?tree=Try&rev=86550fed3486 almost green on try.
Attachment #710222 - Attachment is obsolete: true
Attachment #710387 - Flags: review+
Assignee

Updated

6 years ago
Keywords: checkin-needed
Depends on: 838567
https://hg.mozilla.org/mozilla-central/rev/7f82f74de11f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Doesn't this need to be marked "concrete: False" in workers too?
You need to log in before you can comment on or make changes to this bug.