1058632 - webworker's importScripts() doesn't throw NETWORK_ERROR

Status: RESOLVED INVALID

(Core :: DOM: Workers, defect, P5)

Description

[jakobmaier42]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.8 Safari/537.36

Steps to reproduce:

Call 
    "importScripts.apply(self, ["this_url_does_not_exist"]);" 
or just
    "importScripts("this_url_does_not_exist");" 
within a webworker (dedicated worker).


Actual results:

Nothing, the script continued as nothing would have happened


Expected results:

According to the mozilla guides, this should throw a network_error:
https://developer.mozilla.org/de/docs/Web/Guide/Performance/Using_web_workers#Importing_scripts_and_libraries

Comment 1

[jakobmaier42]

Note that in the example I tested I imported several files, and some of them do exist. But this shouldn't be relevant.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Was this from an HTTP site?

I bet workers don't check for HTTP error pages and just assume that if they got data everything is good...

Comment 3

[jakobmaier42]

Actually it's from an https-page (self-signed), and the console shows the 404-not-found errors.

A difference to Chrome is, that firefox tries to load all files (and prints lots of 404's in the console), while chrome aborts the importing as soon as it found the first missing file.

Comment 4

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

We do check http results, http://mxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#440

Is that insufficient somehow?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Ah, I see.

So the issue is that all we do with loadInfo.mLoadResult is report it to console in ScriptExecutorRunnable::WorkerRun.  We don't convert it into an exception to be thrown from importScripts.

Comment 6

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

Yeah, looks that way. http://mxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#736

Comment 7

[Andrea Marchesini [:baku]]

> So the issue is that all we do with loadInfo.mLoadResult is report it to
> console in ScriptExecutorRunnable::WorkerRun.  We don't convert it into an
> exception to be thrown from importScripts.

Actually we do this:

https://mxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#765

that throws an exception.

Comment 8

[Boris Zbarsky [:bzbarsky]]

No, it reports an error.  To the console.  Via the error reporter.  It doesn't create an exception to throw up the callstack...  (Except in the Throw() case, perhaps, which I really hope we never hit, since nothing here would return false from the actual jsapi entry point!)

Comment 9

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

Sorry, so the way this is supposed to work is that importScripts() blocks in a sync loop while all the network stuff happens: https://mxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#856

Then, each thing gets loaded by necko. If the load fails we're supposed to report here: https://mxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#765

The subtle thing there is that we exit early and don't set |loadInfo.mExecutionResult| to true. That means the final end result of importScripts() will be to throw an exception: https://mxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#802 which calls https://mxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#827 which passes |false| as the result.

That |false| result should be returned here: http://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#4765 and we should throw the exception.

So clearly something isn't working as it's supposed to in this case, but the plumbing is all still in place I think.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Ben, thank you for explaining all that.  So I finally sat down and wrote a test of my own:

  https://web.mit.edu/bzbarsky/www/testcases/workers/test-importscripts-404.html

and I can't reproduce the "firefox tries to load all files (and prints lots of 404's in the console)" bit, nor the "no exception thrown" bit.

The exception thrown is not a NetworkError, but that's not surprising given what scriptloader::ReportLoadError is doing.

Reporter, how does the behavior on my testcase differ from what you're seeing on your testcase?

Comment 11

[jakobmaier42]

The testcase you created works for me, the exception is created.
However, my testcase still doesn't throw exceptions and I don't see any major difference.

I called it with "importScripts(self, "./../../path/to/script1.js", "./../../path/to/script2.js");", and want to load about ~15 scripts with one call. Only one of the scripts does not exist (I think the missing script is somewhere in the middle of the list).

Comment 12

[jakobmaier42]

Sorry, of course I called it with "importScripts.apply(self, ...);"

Comment 13

[Boris Zbarsky [:bzbarsky]]

Can you possibly link to your testcase?  It's hard to debug something I can't reproduce...

Comment 14

[Boris Zbarsky [:bzbarsky]]

Still trying to get hold of a testcase that actually shows the problem.  We have a bunch of tests in our test suite now testing that importScripts throws NETWORK_ERROR as needed... and I still haven't been able to reproduce this issue as reported.

Comment 15

[Jens Stutte [:jstutte]]

As the reporter did not come back with any further information, we'll close this now.