857393 - XHR in Web Workers fails to throw XMLHttpRequest::Send failures to callers (ex: NS_ERROR_FILE_NOT_FOUND), exception may be reported erroneously elsewhere

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

Attachment: fix as discussed with bent; modify aRV

It is possible for (non-worker) XMLHttpRequest::Send to return a non-NS_OK value.  For example, in bug 827243, NS_ERROR_FILE_NOT_FOUND can be returned because AsyncOpen may be able to resolve that a file does not exist in an app:// protocol jar in a synchronous fasion.

(worker) XMLHttpRequest::SendInternal currently returns when !runnable->Dispatch(cx) without modifying aRv, and as a result the error is not propagated to the caller.  However, JS_SetPendingException does get called, so an exception will be tracked on the context, and can end up showing up in misleading places.  For example, :jrburke observed that we might observe the error in importScripts if we tried to perform a synchronous XHR request.

The attached patch created with the guidance of bent resolves the problem affecting the gaia e-mail app.  The specific diff provided is for mozilla-central with crazy context, but the patch applies to mozilla-b2g18 (at least with a smaller context).

I have pushed the patch to try at:
https://tbpl.mozilla.org/?tree=Try&rev=1757a26ab7b4


This blocks the tef+ bug 814257 to move most of the e-mail app's backend to a worker thread and should therefore be tracked as a blocker and allowed to uplift to v1.0.1 and v1-train.  It's a 1-line fix to properly propagate an error code, so I would argue that it is low risk, although bent can be more authoritative on the matter.

Comment 1

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

Comment on attachment 732617 [details] [diff] [review]
fix as discussed with bent; modify aRV

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

r=me. Sorry about that!

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e418e5123168

Comment 3

[Daniel Coloma:dcoloma]

tef+, blocks a blocker

Comment 4

[Ryan VanderMeulen [:RyanVM]]

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

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

uplifted:
https://hg.mozilla.org/releases/mozilla-b2g18/rev/68caf3b9d213
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/5ace431c0dd5

Comment 6

[Tony Chung [:tchung]]

Andrew, can you comment on how QA can verify this patch?

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Tony Chung [:tchung] from comment #6)
> Andrew, can you comment on how QA can verify this patch?

I wouldn't specifically test this patch, but you want to make sure you have a moztrap case that covers creating an account that's not one of the domain names in here: https://github.com/mozilla-b2g/gaia/tree/master/apps/email/autoconfig

Without the patch, that will fail *when running with the back-end on worker-thread from bug 814257*.