930924 - Cannot open a worker from xpcshell-test

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Any attempt to call |new Worker| or |new ChromeWorker| from a xpcshell test results in a "malformed uri" error.
The same call in a jsm called from a worker works.

This complicates a lot writing tests for worker-based libraries.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Opening workers from xpcshell

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Bent, is there a way you could review this patch quickly?
It's a trivial patch and that bug is blocking some of my highest priority work.

Comment 3

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

Comment on attachment 822321 [details] [diff] [review]
Opening workers from xpcshell

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

::: dom/workers/WorkerPrivate.cpp
@@ +3431,4 @@
>          rv = NS_NewURI(getter_AddRefs(loadInfo.mBaseURI),
> +                       fileName);
> +
> +        // ...however, in a few cases (e.g. xpcshell), fileName is a file path

XPCShell is really the only case, right?

@@ +3433,5 @@
> +
> +        // ...however, in a few cases (e.g. xpcshell), fileName is a file path
> +        if (rv == NS_ERROR_MALFORMED_URI) {
> +          nsCOMPtr<nsIFile> scriptFile = do_CreateInstance("@mozilla.org/file/local;1");
> +          MOZ_ASSERT(scriptFile);

I'm not comfortable asserting anything that comes out of the component manager. Please just null check and return an error if it fails.

@@ +3436,5 @@
> +          nsCOMPtr<nsIFile> scriptFile = do_CreateInstance("@mozilla.org/file/local;1");
> +          MOZ_ASSERT(scriptFile);
> +
> +          nsAutoCString scriptPath(fileName);
> +          rv = scriptFile->InitWithNativePath(scriptPath);

InitWithPath please. (IIRC InitWithNativePath has all sorts of problems). You can just pass NS_ConvertUTF8toUTF16(filename) as the arg, no need for the 'scriptPath' variable.

::: dom/workers/test/xpcshell/test_workers.js
@@ +3,5 @@
> +
> +Components.utils.import("resource://gre/modules/Promise.jsm");
> +
> +// Worker must be loaded from a chrome:// uri, not a file://
> +// uri, so we first need to load it.

Wait, I thought the whole point here was so that you could load a Worker from a file:// uri? Why is the chrome path needed?

Comment 4

[David Teller [:Yoric] - still alive but not very active]

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #3)
> XPCShell is really the only case, right?

I think so.

> @@ +3436,5 @@
> > +          nsCOMPtr<nsIFile> scriptFile = do_CreateInstance("@mozilla.org/file/local;1");
> > +          MOZ_ASSERT(scriptFile);
> > +
> > +          nsAutoCString scriptPath(fileName);
> > +          rv = scriptFile->InitWithNativePath(scriptPath);
> 
> InitWithPath please. (IIRC InitWithNativePath has all sorts of problems).
> You can just pass NS_ConvertUTF8toUTF16(filename) as the arg, no need for
> the 'scriptPath' variable.

Sure. Do we have any documentation regarding the encoding used by |fileName|?

> ::: dom/workers/test/xpcshell/test_workers.js
> @@ +3,5 @@
> > +
> > +Components.utils.import("resource://gre/modules/Promise.jsm");
> > +
> > +// Worker must be loaded from a chrome:// uri, not a file://
> > +// uri, so we first need to load it.
> 
> Wait, I thought the whole point here was so that you could load a Worker
> from a file:// uri? Why is the chrome path needed?

We want to load a Worker from a file (turned into a file:// uri by the above patch). Our security policies are not 100% clear to me, but it is my understanding (confirmed by experience) that launching from file:// does not give us the necessary authorization to open a Worker on file://. However, it gives us the necessary authorization to open a Worker on chrome://.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Attachment: Opening workers from xpcshell, v2

Comment 6

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

Comment on attachment 824612 [details] [diff] [review]
Opening workers from xpcshell, v2

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

::: dom/workers/WorkerPrivate.cpp
@@ +3431,4 @@
>          rv = NS_NewURI(getter_AddRefs(loadInfo.mBaseURI),
> +                       fileName);
> +
> +        // ...however, in a few cases (e.g. xpcshell), fileName is a file path

On Windows I never hit your code. NS_NewURI "works" for XPCShell, it parses the filename as {scheme: "C", path: "/Home/src/..."}. I think we probably want to do the file path code first and then try a generic URI if that fails?

::: toolkit/components/workerloader/moz.build
@@ +12,5 @@
> +
> +EXTRA_JS_MODULES += [
> +    'require.js',
> +    'tester.jsm',
> +    'worker_tester.js'

tester.jsm and worker_tester.js are not in this patch, but they don't look required either. This broke the build for me but removing them allowed the tests to run.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #6)
> Comment on attachment 824612 [details] [diff] [review]
> Opening workers from xpcshell, v2
> 
> Review of attachment 824612 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/WorkerPrivate.cpp
> @@ +3431,4 @@
> >          rv = NS_NewURI(getter_AddRefs(loadInfo.mBaseURI),
> > +                       fileName);
> > +
> > +        // ...however, in a few cases (e.g. xpcshell), fileName is a file path
> 
> On Windows I never hit your code. NS_NewURI "works" for XPCShell, it parses
> the filename as {scheme: "C", path: "/Home/src/..."}. I think we probably
> want to do the file path code first and then try a generic URI if that fails?

Good point.


> ::: toolkit/components/workerloader/moz.build
> @@ +12,5 @@
> > +
> > +EXTRA_JS_MODULES += [
> > +    'require.js',
> > +    'tester.jsm',
> > +    'worker_tester.js'
> 
> tester.jsm and worker_tester.js are not in this patch, but they don't look
> required either. This broke the build for me but removing them allowed the
> tests to run.

Ah, my bad.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Attachment: Opening workers from xpcshell, v3

Applied feedback.
Try: https://tbpl.mozilla.org/?tree=Try&rev=76ce2c4167c5

Comment 9

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

Comment on attachment 824842 [details] [diff] [review]
Opening workers from xpcshell, v3

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

::: dom/workers/WorkerPrivate.cpp
@@ +3444,5 @@
> +        rv = scriptFile->InitWithPath(NS_ConvertUTF8toUTF16(fileName));
> +        if (NS_SUCCEEDED(rv)) {
> +          rv = NS_NewFileURI(getter_AddRefs(loadInfo.mBaseURI),
> +                             scriptFile);
> +        } else if (rv == NS_ERROR_FILE_UNRECOGNIZED_PATH) {

I think this shouldn't be 'else if'. We should fall back to NS_NewURI if either the InitWithPath or NS_NewFileURI fails.

@@ +3453,5 @@
> +        }
> +        if (NS_FAILED(rv)) {
> +          return rv;
> +        }
> +

Nit: Kill this extra line

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Attachment: Opening workers from xpcshell, v4

Here we go

Comment 11

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

Comment on attachment 827383 [details] [diff] [review]
Opening workers from xpcshell, v4

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

This looks good to me, thanks! One nit below:

::: dom/workers/WorkerPrivate.cpp
@@ +3447,5 @@
> +                             scriptFile);
> +        }
> +        if (NS_FAILED(rv)) {
> +          // At this stage, there are chances that the path is actually
> +          // a URI

Nit: This comment is wrong, in almost all cases the path is a URI. I'd just remove it.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Attachment: Opening workers from xpcshell, v5

Changed comment as requested
Try: https://tbpl.mozilla.org/?tree=Try&rev=e1056d7fb67e

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd8b12b85220

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dd8b12b85220