827803 - [OS.File] Refine exception transmission across threads

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Owen, are you still working on this bug?

Comment 2

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 3

[Owen Coutts [:tallOwen]]

Sorry for not unassigning earlier!

Comment 4

[Sahil Chelaramani]

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?

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Hi Sahil Chelaramani, sorry for not seeing your message earlier.
Are you still interested in this bug?

Comment 6

[Sahil Chelaramani]

Yes! I'll still be happy to work on this!!

Comment 7

[David Teller [:Yoric] - still alive but not very active]

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 :)

Comment 8

[Sahil Chelaramani]

Attachment: Added support for listed exceptions

I've added support for the listed errors to the file osfile_async_worker.js. Please review.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

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 });

Comment 10

[David Teller [:Yoric] - still alive but not very active]

So, Sahil, how are things going? Do you need further assistance?

Comment 11

[Sahil Chelaramani]

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.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Any news?

Comment 13

[Peiyong Lin[:lpy](UTC+8)]

Hi Sahil, could you please let know whether you are still working on this bug or not?

Comment 14

[Sahil Chelaramani]

No I am not currently working on this. I'm sorry for Not Unassigning earlier.

Comment 15

[Peiyong Lin[:lpy](UTC+8)]

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)|?

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Indeed, that's the idea.

Comment 17

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug827803.patch

Thank you. :)

Comment 18

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 19

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug827803-V2.patch

Thank you. Patch updated.

Comment 20

[David Teller [:Yoric] - still alive but not very active]

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?

Comment 21

[Peiyong Lin[:lpy](UTC+8)]

How could I catch TypeError from the main thread? I could only catch TypeError from the worker side. Any help please?

Comment 22

[David Teller [:Yoric] - still alive but not very active]

(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"?

Comment 23

[Peiyong Lin[:lpy](UTC+8)]

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.

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Well, osfile_async_front.jsm is supposed to turn it back into a TypeError, isn't it?

Comment 25

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug827803-V3.patch

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.

Comment 26

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug827803-V4.patch

Oh I forgot to clean some code.

Comment 27

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 28

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug827803-V5.patch

Comment 29

[David Teller [:Yoric] - still alive but not very active]

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

Comment 30

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug827803-V6.patch

Thank you :)

Comment 31

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug827803-V7.patch

Forgot to refresh the patch.

Comment 32

[David Teller [:Yoric] - still alive but not very active]

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);

Comment 33

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug827803-Final.patch

Thank you for your help :)

Comment 34

[Peiyong Lin[:lpy](UTC+8)]

Try result looks good.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/e7010796fdc0

Comment 36

[Ryan VanderMeulen [:RyanVM]]

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