We need to transmit errors between threads.

Comment on attachment 649585 [details] [diff] [review] Unix errors Review of attachment 649585 [details] [diff] [review]: ----------------------------------------------------------------- Hardly anything to complain about! ::: toolkit/components/osfile/osfile_unix_allthreads.jsm @@ +136,5 @@ > + /** > + * Deserialize a message back to an instance of OSError > + */ > + OSError.fromMsg = function fromMsg(msg) { > + return new OSError(msg.operation, msg.unixErrno); Are we doing epsilon error checking everywhere in the deserialization process?

Comment on attachment 649586 [details] [diff] [review] Windows errors Review of attachment 649586 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/osfile/osfile_win_allthreads.jsm @@ +138,5 @@ > + */ > + OSError.toMsg = function toMsg(error) { > + return { > + operation: error.operation, > + winLastError: error.winLastError Nit: indentation issues.

I do wonder whether |toMsg| should be defined on the prototype, or whether that would represent a major break from all other |toMsg| functions elsewhere.

Comment on attachment 649587 [details] [diff] [review] Companion testsuite Review of attachment 649587 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/osfile/tests/mochi/worker_test_osfile_comms.js @@ +98,5 @@ > + check: function check_error(candidate, prefix) { > + ok(candidate instanceof OS.File.Error, > + prefix + "Error is an OS.File.Error"); > + ok(candidate.unixErrno == 1 || candidate.winLastError == 1, > + prefix + "Error code is correct"); What do you think about providing an errorNumber getter for OS.File.Error so that we can avoid platform-specific details here?

(In reply to Nathan Froyd (:froydnj) from comment #6) > Comment on attachment 649585 [details] [diff] [review] > Unix errors > > Review of attachment 649585 [details] [diff] [review]: > ----------------------------------------------------------------- > > Hardly anything to complain about! > > ::: toolkit/components/osfile/osfile_unix_allthreads.jsm > @@ +136,5 @@ > > + /** > > + * Deserialize a message back to an instance of OSError > > + */ > > + OSError.fromMsg = function fromMsg(msg) { > > + return new OSError(msg.operation, msg.unixErrno); > > Are we doing epsilon error checking everywhere in the deserialization > process? I am not familiar with that term. What does this mean?

(In reply to Nathan Froyd (:froydnj) from comment #9) > Comment on attachment 649587 [details] [diff] [review] > Companion testsuite > > Review of attachment 649587 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/components/osfile/tests/mochi/worker_test_osfile_comms.js > @@ +98,5 @@ > > + check: function check_error(candidate, prefix) { > > + ok(candidate instanceof OS.File.Error, > > + prefix + "Error is an OS.File.Error"); > > + ok(candidate.unixErrno == 1 || candidate.winLastError == 1, > > + prefix + "Error code is correct"); > > What do you think about providing an errorNumber getter for OS.File.Error so > that we can avoid platform-specific details here? Since the constants error number are platform-specific, I do not think there would be any gain. OSError.prototype offers a number of |becauseXXX| getters (which still need to be fleshed out a little) that can be used to portably determine the reason of an error. Here, the arbitrary choice of "error 1" is just to keep the test simple.

(In reply to Nathan Froyd (:froydnj) from comment #8) > I do wonder whether |toMsg| should be defined on the prototype, or whether > that would represent a major break from all other |toMsg| functions > elsewhere. This is definitely possible. For many data structures, I have no choice, and I need |toMsg| to be a function rather than a method, either because I cannot extend the values or because I need to cope with |null|. For errors, I just kept it here for homogeneity. I would rather maintain this homogeneity but I have no strong objection to changing this if you want me to.

(In reply to David Rajchenbach Teller from comment #10) > > ::: toolkit/components/osfile/osfile_unix_allthreads.jsm > > @@ +136,5 @@ > > > + /** > > > + * Deserialize a message back to an instance of OSError > > > + */ > > > + OSError.fromMsg = function fromMsg(msg) { > > > + return new OSError(msg.operation, msg.unixErrno); > > > > Are we doing epsilon error checking everywhere in the deserialization > > process? > > I am not familiar with that term. What does this mean? Sorry, thought you might have been! "epsilon" is the generic mathematical term for "a quantity arbitrarily close to zero" (In reply to David Rajchenbach Teller from comment #11) > > ::: toolkit/components/osfile/tests/mochi/worker_test_osfile_comms.js > > @@ +98,5 @@ > > > + check: function check_error(candidate, prefix) { > > > + ok(candidate instanceof OS.File.Error, > > > + prefix + "Error is an OS.File.Error"); > > > + ok(candidate.unixErrno == 1 || candidate.winLastError == 1, > > > + prefix + "Error code is correct"); > > > > What do you think about providing an errorNumber getter for OS.File.Error so > > that we can avoid platform-specific details here? > > Since the constants error number are platform-specific, I do not think there > would be any gain. OSError.prototype offers a number of |becauseXXX| getters > (which still need to be fleshed out a little) that can be used to portably > determine the reason of an error. > > Here, the arbitrary choice of "error 1" is just to keep the test simple. Indeed. I was concerned about the (quite outside) possibility that the Unix code might define winLastError or vice versa. But this is probably not a large enough detail to care about.

(In reply to Nathan Froyd (:froydnj) from comment #13) > > > Are we doing epsilon error checking everywhere in the deserialization > > > process? > > > > I am not familiar with that term. What does this mean? > > Sorry, thought you might have been! "epsilon" is the generic mathematical > term for "a quantity arbitrarily close to zero" I am quite familiar with epsilon, just not with epsilon error checking :) $$\forall x \in D, \forall y \in D, \forall \epsilon > 0, \exists \eta > 0, dist1(x, y) < \eta \Rightarrow dist2(f(x), f(y)) < \epsilon$$ maybe? :) > > Since the constants error number are platform-specific, I do not think there > > would be any gain. OSError.prototype offers a number of |becauseXXX| getters > > (which still need to be fleshed out a little) that can be used to portably > > determine the reason of an error. > > > > Here, the arbitrary choice of "error 1" is just to keep the test simple. > > Indeed. I was concerned about the (quite outside) possibility that the Unix > code might define winLastError or vice versa. But this is probably not a > large enough detail to care about. ok

(In reply to David Rajchenbach Teller from comment #14) > (In reply to Nathan Froyd (:froydnj) from comment #13) > > > > Are we doing epsilon error checking everywhere in the deserialization > > > > process? > > > > > > I am not familiar with that term. What does this mean? > > > > Sorry, thought you might have been! "epsilon" is the generic mathematical > > term for "a quantity arbitrarily close to zero" > > I am quite familiar with epsilon, just not with epsilon error checking :) > $$\forall x \in D, \forall y \in D, \forall \epsilon > 0, \exists \eta > 0, > dist1(x, y) < \eta \Rightarrow dist2(f(x), f(y)) < \epsilon$$ > maybe? :) Yes, something like that. :) I guess I should have more clearly stated it as "an epsilon amount of error checking". (In reply to David Rajchenbach Teller from comment #12) > (In reply to Nathan Froyd (:froydnj) from comment #8) > > I do wonder whether |toMsg| should be defined on the prototype, or whether > > that would represent a major break from all other |toMsg| functions > > elsewhere. > > This is definitely possible. For many data structures, I have no choice, and > I need |toMsg| to be a function rather than a method, either because I > cannot extend the values or because I need to cope with |null|. For errors, > I just kept it here for homogeneity. > > I would rather maintain this homogeneity but I have no strong objection to > changing this if you want me to. Homogeneity is a good thing; if we can't make the switch in a majority of the places, let's just keep it as-is.

try: https://tbpl.mozilla.org/?tree=Try&rev=52d842713958

Tweaking landing order.

(In reply to David Rajchenbach Teller from comment #19) > try: https://tbpl.mozilla.org/?tree=Try&rev=52d842713958 Green on Try. Had to un-bitrot it a bit first. Please double-check to confirm that I didn't do anything dumb. https://hg.mozilla.org/integration/mozilla-inbound/rev/4387edee4099 https://hg.mozilla.org/integration/mozilla-inbound/rev/03c71c9f71a9 https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2ccd16a2c0

https://hg.mozilla.org/mozilla-central/rev/4387edee4099 https://hg.mozilla.org/mozilla-central/rev/03c71c9f71a9 https://hg.mozilla.org/mozilla-central/rev/eb2ccd16a2c0

Ryan: looks good to me.