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

RESOLVED FIXED in mozilla23

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: asuth, Assigned: asuth)

Tracking

Trunk
mozilla23
x86_64
Linux
Points:
---

Firefox Tracking Flags

(blocking-b2g:tef+, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
Created attachment 732617 [details] [diff] [review]
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.
Attachment #732617 - Flags: review?(bent.mozilla)
(Assignee)

Updated

6 years ago
See Also: → bug 827243
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!
Attachment #732617 - Flags: review?(bent.mozilla) → review+
tef+, blocks a blocker
blocking-b2g: tef? → tef+
https://hg.mozilla.org/mozilla-central/rev/e418e5123168
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(Assignee)

Comment 5

6 years ago
uplifted:
https://hg.mozilla.org/releases/mozilla-b2g18/rev/68caf3b9d213
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/5ace431c0dd5
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → verified disabled
status-b2g18-v1.0.1: --- → fixed
(Assignee)

Updated

6 years ago
status-b2g18-v1.0.0: verified disabled → wontfix

Comment 6

6 years ago
Andrew, can you comment on how QA can verify this patch?
(Assignee)

Comment 7

6 years ago
(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*.
You need to log in before you can comment on or make changes to this bug.