Closed Bug 827803 Opened 10 years ago Closed 9 years ago

[OS.File] Refine exception transmission across threads

Categories

(Toolkit :: OS.File, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: Yoric, Assigned: lpy)

Details

(Whiteboard: [Async:ready][mentor=Yoric][lang=js])

Attachments

(1 file, 8 obsolete files)

According to gps in bug 827206, some operations of OS.File reject with a WorkerErrorEvent. That's not normal. They should reject with OS.File.Error.
Whiteboard: [mentor=Yoric][lang=js]
Assignee: nobody → owen
Owen, are you still working on this bug?
Flags: needinfo?(owen)
Whiteboard: [mentor=Yoric][lang=js] → [mentor=Yoric][lang=js][Async]
At the moment, osfile_async_worker.js uses special treatment for exceptions that are either
- StopIteration; or
- instances of OS.File.Error.

Actually, we should also add special treatment for exceptions that are instances of e.g.:
- EvalError
- InternalError
- RangeError
- ReferenceError
- SyntaxError
- TypeError
- URIError

For these exceptions, we need to:
- worker-side;
  - store the kind of exception (i.e. the constructor name);
  - store the stack;
  - send the message, file name, line number and stack;
- main thread-side;
  - rebuild an instance of the constructor, with the appropriate message, line number and stack.
Sorry for not unassigning earlier!
Assignee: owen → nobody
Flags: needinfo?(owen)
Hey,
I'd like to work on this bug, but I'm new here,so could you give me some more information about what exactly needs to be done and resources that I might need to modify to achieve it?
Hi Sahil Chelaramani, sorry for not seeing your message earlier.
Are you still interested in this bug?
Flags: needinfo?(Sahilc.2200)
Yes! I'll still be happy to work on this!!
Flags: needinfo?(Sahilc.2200)
Great :)
Let me give you some background on this bug.
This bug is about OS.File, our JavaScript library for handling files. You will need to familiarize yourself a little with Workers ( https://developer.mozilla.org/en-US/docs/DOM/Using_web_workers ) and Promise ( https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Promise.jsm ).

With OS.File, everything is executed on a dedicated Worker. Unfortunately, by default, when an error is thrown in the Worker, it is transformed in a WorkerErrorEvent, which is not very convenient.

The objective of this bug is to rewrite the parts of the code that handle errors to improve this situation, as detailed in comment 2. You may find the code that handles errors here: http://dxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_async_worker.js?from=osfile_async_worker.js#l77 You will need to extend that code to convert errors into messages.

You will also need to modify the following code: http://dxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_async_front.jsm?from=osfile_async_front.jsm#l147 to receive the messages and turn them back into errors.

If you have any question, you may find me (and other mentors) on irc.mozilla.org, channel #introduction.

Have fun :)
Assignee: nobody → Sahilc.2200
Summary: [OS.File] Wrong exception → [OS.File] Refine exception transmission across threads
I've added support for the listed errors to the file osfile_async_worker.js. Please review.
Attachment #801173 - Flags: review?(dteller)
Comment on attachment 801173 [details] [diff] [review]
Added support for listed exceptions

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

Good start.
Now, you'll need to modify osfile_async_front.jsm to use the information sent by the worker and rebuild exceptions. We will also need to find a nice way to test this.

::: toolkit/components/osfile/modules/osfile_async_worker.js
@@ +103,5 @@
> +         LOG("Sending back TypeError");
> +         self.postMessage({RangeError: true, id:id, durationMs: durationMs});
> +       } else if (exn == URIError) {
> +         LOG("Sending back URIError");
> +         self.postMessage({URIError: true, id:id, durationMs: durationMs});

Rather than all these possible fields |RangeError|, |EvalError|, ... I would add a single field |exn| that contains a string which may be "RangeError", "EvalError", etc.

Also, let's take the opportunity to send the exception message, file name and line number, as follows:

self.postMessage({ exn: exn.name, message: exn.message, fileName: exn.fileName, lineNumber, exn.lineNumber, id: id, durationMs: durationMs });
Attachment #801173 - Flags: review?(dteller) → feedback+
So, Sahil, how are things going? Do you need further assistance?
Flags: needinfo?(Sahilc.2200)
Sorry for the delay. Ran into a couple of issues, but now working on sorting them out. Will submit an updated patch over the weekend.
Flags: needinfo?(Sahilc.2200)
Any news?
Flags: needinfo?(Sahilc.2200)
Hi Sahil, could you please let know whether you are still working on this bug or not?
No I am not currently working on this. I'm sorry for Not Unassigning earlier.
Flags: needinfo?(Sahilc.2200)
Assignee: Sahilc.2200 → nobody
Whiteboard: [mentor=Yoric][lang=js][Async] → [Async:ready][mentor=Yoric][lang=js]
Assignee: nobody → pylaurent1314
Hi Yoric. Just want to ask to make it clear to me. The error message on the main thread-side should be the same as the old one? Like |new Error(error.message, error.fileName, error.lineNumber)|?
Indeed, that's the idea.
Attached patch bug827803.patch (obsolete) — Splinter Review
Thank you. :)
Attachment #801173 - Attachment is obsolete: true
Attachment #8362233 - Flags: review?(dteller)
Comment on attachment 8362233 [details] [diff] [review]
bug827803.patch

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

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +211,5 @@
>              throw StopIteration;
>            }
>            throw new Error(message, error.filename, error.lineno);
> +        } else {
> +          throw new Error(error.message, error.fileName, error.lineNumber);

No, we want to throw a TypeError if the original error was a TypeError, a RangeError if it was a RangeError, etc.

::: toolkit/components/osfile/modules/osfile_async_worker.js
@@ +93,5 @@
>       // instances of |OS.File.Error|)
>       self.postMessage({fail: exports.OS.File.Error.toMsg(exn), id:id, durationMs: durationMs});
> +   } else if (exn == EvalError || exn == InternalError || exn == RangeError ||
> +              exn == ReferenceError || exn == SyntaxError || exn == TypeError ||
> +              exn == URIError) {

That's not going to work.
What you can do is check whether |exn.constructor.name| is in |["EvalError", ...]|.

@@ +94,5 @@
>       self.postMessage({fail: exports.OS.File.Error.toMsg(exn), id:id, durationMs: durationMs});
> +   } else if (exn == EvalError || exn == InternalError || exn == RangeError ||
> +              exn == ReferenceError || exn == SyntaxError || exn == TypeError ||
> +              exn == URIError) {
> +     exn.name = exn.constructor.name;

Please don't modify the exception by side-effect.
Attachment #8362233 - Flags: review?(dteller) → feedback+
Attached patch bug827803-V2.patch (obsolete) — Splinter Review
Thank you. Patch updated.
Attachment #8362233 - Attachment is obsolete: true
Attachment #8363001 - Flags: review?(dteller)
Comment on attachment 8363001 [details] [diff] [review]
bug827803-V2.patch

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

I believe it would be useful if you could start by writing a test.
You can patch test_exception.js to ensure that the TypeError thrown on the worker ends up a TypeError on the main thread.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +228,1 @@
>          }

I'm sure that you can do better than this sequence of |else if|, for instance by having a global object with fields "EvalError", etc.

::: toolkit/components/osfile/modules/osfile_async_worker.js
@@ +92,5 @@
>       // (deserialization ensures that we end up with OS-specific
>       // instances of |OS.File.Error|)
>       self.postMessage({fail: exports.OS.File.Error.toMsg(exn), id:id, durationMs: durationMs});
> +   } else if (exn.constructor.name in ["EvalError", "InternalError", "RangeError",
> +                                       "ReferenceError", "SyntaxError", "TypeError", "URIError"]) {

Note:
 "foo" in ["foo", "bar"]
produces |false|

This is probably not what you want.

Also, this will recreate the array each time you enter the function. Since this array is a constant, you should be able to move it to the global scope and save work for the gc.

@@ +95,5 @@
> +   } else if (exn.constructor.name in ["EvalError", "InternalError", "RangeError",
> +                                       "ReferenceError", "SyntaxError", "TypeError", "URIError"]) {
> +     LOG("Sending back", exn.constructor.name);
> +     self.postMessage({exn: exn.constructor.name, message: exn.message, fileName: exn.fileName,
> +                       lineNumber: exn.lineNumber, id: id, durationMs: durationMs});

Nit: Could you keep the lines shorter?
Attachment #8363001 - Flags: review?(dteller) → feedback+
How could I catch TypeError from the main thread? I could only catch TypeError from the worker side. Any help please?
(In reply to Peiyong Lin[:lpy](UTC-8) from comment #21)
> How could I catch TypeError from the main thread? I could only catch
> TypeError from the worker side. Any help please?

Oh, right. I forgot that errors don't cross compartments.
In test_exception.js, can you just check that exn.constructor.name == "TypeError"?
From the patch, The worker side will post the error message to the main thread, it will not throw the TypeError exception. So I could not catch it any more in test_exception.js.
Well, osfile_async_front.jsm is supposed to turn it back into a TypeError, isn't it?
Attached patch bug827803-V3.patch (obsolete) — Splinter Review
Finally I figured out what I was wrong. The {exn: exn.constructor.name, message: exn.message, blahblah} should be in the |fail| field, so in the worker side, it should be |post({fail: {exn: exn.constructor.name, blahblah}, blahblah});| Thank you very much for your help :)

Also, when using |new TypeError(error.message, error.fileName, error.lineNumber)|, I can not see the output has the file name or line number. But |new TypeError(error.message + " in " + error.fileName + " of line " + error.lineNumber)| will output the file name and line number. I don't know whether it is necessary or not.
Attachment #8363001 - Attachment is obsolete: true
Attachment #8364255 - Flags: review?(dteller)
Attached patch bug827803-V4.patch (obsolete) — Splinter Review
Oh I forgot to clean some code.
Attachment #8364255 - Attachment is obsolete: true
Attachment #8364255 - Flags: review?(dteller)
Attachment #8364257 - Flags: review?(dteller)
Comment on attachment 8364257 [details] [diff] [review]
bug827803-V4.patch

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

Looks like we're almost done.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +185,5 @@
> +      },
> +      URIError: function(error) {
> +        throw new URIError(error.message, error.fileName, error.lineNumber);
> +      }
> +    };

We don't want the object to be recreated each time we reach that line of code. Please make this a global object.

::: toolkit/components/osfile/modules/osfile_async_worker.js
@@ +70,5 @@
> +     ReferenceError: "ReferenceError",
> +     SyntaxError: "SyntaxError",
> +     TypeError: "TypeError",
> +     URIError: "URIError"
> +   };

Please make this a global object. Also, since it's a constant, the name should be upper-case – and some documentation would be useful.
Attachment #8364257 - Flags: review?(dteller) → feedback+
Attached patch bug827803-V5.patch (obsolete) — Splinter Review
Attachment #8364257 - Attachment is obsolete: true
Attachment #8364433 - Flags: review?(dteller)
Comment on attachment 8364433 [details] [diff] [review]
bug827803-V5.patch

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

Almost good. I'd like a few minor changes before we land this.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +58,5 @@
>  Cu.import("resource://gre/modules/TelemetryStopwatch.jsm", this);
>  Cu.import("resource://gre/modules/AsyncShutdown.jsm", this);
>  
> +// Exception handler.
> +let exceptionThrows = {

Let's make this

/**
 * Constructors for decoding standard exceptions
 * received from the worker.
 */
const EXCEPTION_CONSTRUCTORS = {

@@ +60,5 @@
>  
> +// Exception handler.
> +let exceptionThrows = {
> +  EvalError: function(error) {
> +    throw new EvalError(error.message, error.fileName, error.lineNumber);

For clarity, could you rather return the exception here and throw it where you call the function.

::: toolkit/components/osfile/modules/osfile_async_worker.js
@@ +9,5 @@
>  
>  // Worker thread for osfile asynchronous front-end
>  
> +// Exception names to be posted from the worker thread to main thread
> +let EXCEPTIONNAMES = {

const EXCEPTION_NAMES
Attachment #8364433 - Flags: review?(dteller) → feedback+
Attached patch bug827803-V6.patch (obsolete) — Splinter Review
Thank you :)
Attachment #8364433 - Attachment is obsolete: true
Attachment #8365068 - Flags: review?(dteller)
Attached patch bug827803-V7.patch (obsolete) — Splinter Review
Forgot to refresh the patch.
Attachment #8365068 - Attachment is obsolete: true
Attachment #8365068 - Flags: review?(dteller)
Attachment #8365103 - Flags: review?(dteller)
Comment on attachment 8365103 [details] [diff] [review]
bug827803-V7.patch

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

Looks good.
r=me, but could you make the following trivial change?

Try results: https://tbpl.mozilla.org/?tree=Try&rev=494715f5b06b

::: toolkit/components/osfile/modules/osfile_async_worker.js
@@ +121,5 @@
>       // (deserialization ensures that we end up with OS-specific
>       // instances of |OS.File.Error|)
>       post({fail: exports.OS.File.Error.toMsg(exn), id:id, durationMs: durationMs});
> +   } else if (exn.constructor.name in EXCEPTION_NAMES) {
> +     LOG("Sending back", exn.constructor.name);

Maybe
 LOG("Sending back exception", exn.constructor.name);
Attachment #8365103 - Flags: review?(dteller) → review+
Thank you for your help :)
Attachment #8365103 - Attachment is obsolete: true
Attachment #8365933 - Flags: review+
Try result looks good.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/e7010796fdc0
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [Async:ready][mentor=Yoric][lang=js] → [Async:ready][mentor=Yoric][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/e7010796fdc0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [Async:ready][mentor=Yoric][lang=js][fixed-in-fx-team] → [Async:ready][mentor=Yoric][lang=js]
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.