Open Bug 1309447 Opened 8 years ago Updated 2 years ago

test_intl_on_workers.js doesn't work on Android


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




Tracking Status
firefox52 --- wontfix


(Reporter: m_kato, Assigned: m_kato)




(1 file, 1 obsolete file)

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.
Blocks: 1307164
Attachment #8800065 - Flags: review?(jwalden+bmo)
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.
Attachment #8800065 - Flags: review?(jwalden+bmo)
Summary: Disable test_intl_on_workers.js on Android → test_intl_on_workers.js doesn't work on Android
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).
When fixing xpcshell remote by changing to absolute path, some tests become failure.

But, it seems to be that absolute path is too long.  When path length becomes short, it seems to be successful.
No longer blocks: 1307164
Attachment #8800065 - Attachment is obsolete: true
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.
Attachment #8805007 - Flags: review?(jwalden+bmo)
Blocks: 1215247
Blocks: 1174386
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",
             return false;
+        const char* filePath;
+#ifdef ANDROID
+        char absPath[PATH_MAX];
+        filePath = realpath(filename.ptr(), absPath))
+                   ? absPath
+                   : filename.ptr();
+        filePath = filename.ptr();
         JS::CompileOptions options(cx);
-               .setFileAndLine(filename.ptr(), 1)
+               .setFileAndLine(filePath, 1)
         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 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?
Attachment #8805007 - Flags: review?(jwalden+bmo) → feedback+
: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?
Flags: needinfo?(amarchesini)
> 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:

What about using the URI fallback btw?
Flags: needinfo?(amarchesini)
Depends on: 1332565
Too late for firefox 52, mass-wontfix.
Priority: -- → P5
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.