1309447 - test_intl_on_workers.js doesn't work on Android

Status: NEW

(Core :: JavaScript: Internationalization API, defect, P5)

Description

[Makoto Kato [:m_kato]]

From bug Bug 1307164 comment #6.

Actually, xpcshell on android doesn't work worker test via chrome://.  So xpcshell  test into dom/workers/test/xpcshell/* is disabled.  So we should skip test_intl_on_workers.js on Android's xpcshell test as temporary.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Disable test_intl_on_workers.js on Android

Comment 2

[Makoto Kato [:m_kato]]

Comment on attachment 8800065 [details] [diff] [review]
Disable test_intl_on_workers.js on Android

sorry, I cancel this review.  I found root cause on test infra bug.

Comment 3

[Makoto Kato [:m_kato]]

Load in XPCShellImpl.cpp sets script's filename using options.setFileAndLine.  But this filename isn't absolute path on Android's remote xpcshell test.  When using chrome worker, WorkerPrivate::GetLoadInfo gets this filename.  Since filename isn't absolute path, NS_NewFileURI will return NS_ERROR_MALFORMED_URI.  So all chrome worker test by xpcshell is failure on Android (and remote version).

Comment 4

[Makoto Kato [:m_kato]]

When fixing xpcshell remote by changing to absolute path, some tests become failure.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf0ef167aa2ca06e5f1f08612cd2c729fabcf5dd

But, it seems to be that absolute path is too long.  When path length becomes short, it seems to be successful.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69505f743174e3045c69b609d976f7179b02bc33

Comment 5

[Makoto Kato [:m_kato]]

Attachment: Set relative path as filename

Comment 6

[Makoto Kato [:m_kato]]

Comment on attachment 8805007 [details] [diff] [review]
Set relative path as filename

WorkerPrivate::GetLoadInfo requires absolute filename for javascript file to use Chrome worker.  But xpchsell on Android doesn't use absolute path.  Because some xpcshell tests are failure on our test infra due to too long arguments.

So we should set absolute path for xpcshell to run chrome worker unit test.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8805007 [details] [diff] [review]
Set relative path as filename

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

So in principle this looks fine, with a little tweaking around the edges.  But I'm entirely unsure that this is the right place in the stack to make this adjustment, I don't see an immediate reason why this behavior should be limited to Android only, I'm unclear why it's okay for worker code to assume the worker's filename is an absolute path (or, as fallback, an absolute URL), and generally I'm just not all that confident in the Gecko half of this patch, from an architectural point of view.

Someone else definitely should review this, in addition to me, before it lands -- although whether a DOM workers peer, or maybe jimb or fitzgen as a debugger person who knows what Gecko-specific semantics are imposed on script filenames, or someone else, I'm really not sure.  :-\  I guess maybe start with a DOM workers person and we can escalate from there if they're not sure?

::: js/xpconnect/src/XPCShellImpl.cpp
@@ +368,5 @@
>          JS::CompileOptions options(cx);
> +#ifdef ANDROID
> +        // Chrome worker doesn't work when filename isn't absolute path.
> +        // Remote xpcshell may use relative path since arguments are too long,
> +        // so we get absolute path.

This comment is a little bit choppy.  Assuming this is the right place for this adjustment (see next comment), let's go with something like

"""
Consumers of the JSAPI-supplied filename in Gecko sometimes expect it to be absolute: either a URL or a file path.  For example, DOM worker code queries filename, in order that when a worker script specifies relative paths, they can be resolved with respect to the worker's location.  (See bug 1309447.)  Therefore, make paths absolute before specifying them as script filenames.
"""

...that said.  As I write ^, and as I consider the next review comment: why aren't we making paths absolute on *all* platforms, not just Android?  It seems like it should be at least harmless, possibly consistent with some Gecko-wide requirement that script filenames be absolute paths/URLs, and would simplify documenting an edge case.  And it seems like just because Android's the only platform that's wanting this *so far*, in principle any other platform could next time we turn around.

@@ +375,5 @@
> +            strcpy(absPath, filename.ptr());
> +        }
> +        options.setUTF8(true)
> +               .setFileAndLine(absPath, 1)
> +               .setIsRunOnce(true);

Rather than duplicate code, I'd prefer if you had a single variable you just assigned differently, something like:

diff --git a/js/xpconnect/src/XPCShellImpl.cpp b/js/xpconnect/src/XPCShellImpl.cpp
--- a/js/xpconnect/src/XPCShellImpl.cpp
+++ b/js/xpconnect/src/XPCShellImpl.cpp
@@ -364,11 +364,22 @@ Load(JSContext* cx, unsigned argc, Value
             JS_ReportErrorUTF8(cx, "cannot open file '%s' for reading",
                                filename.ptr());
             return false;
         }
+
+        const char* filePath;
+#ifdef ANDROID
+        char absPath[PATH_MAX];
+        filePath = realpath(filename.ptr(), absPath))
+                   ? absPath
+                   : filename.ptr();
+#else
+        filePath = filename.ptr();
+#endif
+
         JS::CompileOptions options(cx);
         options.setUTF8(true)
-               .setFileAndLine(filename.ptr(), 1)
+               .setFileAndLine(filePath, 1)
                .setIsRunOnce(true);
         JS::Rooted<JSScript*> script(cx);
         JS::Rooted<JSObject*> global(cx, JS::CurrentGlobalOrNull(cx));
         JS::Compile(cx, options, file, &script);

That said, I'm not entirely certain that this is the right place in the stack for this.  Might WorkerPrivate::GetLoadInfo instead be the code that should do this, because it needs an absolute file path?  Or is there some unstated Gecko invariant that script filenames are always either absolute URLs or absolute file paths?

Skimming http://searchfox.org/mozilla-central/search?q=setFileAndLine it does appear that most supplied filenames are absolute (by dint of being URL specs), so supplying something absolute is consistent with that.  But I could be mistaken.  Could you find a DOM/workers peer who can review this for whether this is the right place to make the change?

Comment 8

[Makoto Kato [:m_kato]]

:baku, actually chrome worker doesn't work on Android's xpcshell test.

Due to limitation (arguments are too long) of remote xpcshell test, xpcshell test (on Android's remote only) uses relative path for test file.

Now, WorkerPrivate::GetLoadInfo uses script file name for Base URI.  But since this isn't absolute path, NS_NewFileURI will be failed.  Then, chrome worker on Anroid's remote xpcshell doesn't work.

So could you answer comment #7 by :waldo?, or who is right person for chrome worker?

Comment 9

[Andrea Marchesini [:baku]]

> So could you answer comment #7 by :waldo?, or who is right person for chrome
> worker?

We could extend WorkerPrivate::GetLoadInfo in order to support relative paths.
At the moment we support loading of workers if these are files and then, as a fallback, as URI.
See comments: https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#4516

What about using the URI fallback btw?

Comment 10

[Julien Cristau [:jcristau]]

Too late for firefox 52, mass-wontfix.