1286009 - Distinguish XDR error code.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

Currently, we return various kind of errors from XDR, with no proper way to distinguish an Out-of-memory from a logical error which indicates that we either cannot properly encode/decode script.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Distinguish failure reasons of JS_{En,De}codeScript.

This adds a jsapi function, to let callers of JS_EncodeScript and
JS_DecodeScript know if the failure is an OOM, or any other failure from
which we can continue the execution as-if nothing happened.

Comment 2

[Luke Wagner [:luke]]

I think we could have a somewhat simpler and more explicit API by considering an attempt to encode a script with one of these unsupported features to not be an error but, rather, an alternate result.  Thus you'd have:
  enum class EncodeResult { Serialized, UnserializableScript };
  bool JS_EncodeScript(JSContext*, ..., EncodeResult* result, void** serializedBytes);
such that a false return value means a real error that should be propagated like normal but 'true' means you have to look at the EncodeResult to know if you got serializedBytes or not.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: Distinguish failure reasons of JS_{En,De}codeScript.

I tried to follow as much as possible the suggestion. I prefer to return the
error code as the default return value, as opposed to have an additional
out-param for that.

Unfortunately, for the TranscodeResult, I had to use an "enum" instead of an
"enum class" in order to use bit-masks for checking for generic forms of
failures / errors.  I also need these detailed error messages for the test
suite.

As this patch change the signature of the function, I used MutableHandle for
the out-param of the decode functions.  This added some additional wrapping
in ReadScriptOrFunction.  I will try to remove this glue in a follow-up patch.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8770662 [details] [diff] [review]
Distinguish failure reasons of JS_{En,De}codeScript.

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

Thanks!  Generally looks good.

::: js/src/jsapi.cpp
@@ +6448,2 @@
>      if (!encoder.codeScript(&script))
> +        encoder.postProcessContextErrors(cx);

I think the interface can be simplified a bit to have codeScript()/codeFunction() have done postProcessContextErrors() themselves before returning.

::: js/src/jsapi.h
@@ +5697,4 @@
>  
> +    // A warning message, is set to the message out-param.
> +    TranscodeResult_Warning = 0x100,
> +    TranscodeResult_Warning_BadBuildId =          TranscodeResult_Warning | 0x1,

The name "Warning" might lead people to think that it still worked (and so the *buffer was present).  Perhaps "Failure" and rename "Error" to "Throw" to highlight the distinction?

::: js/xpconnect/loader/mozJSLoaderUtils.cpp
@@ +36,5 @@
> +    if ((code & TranscodeResult_Warning) != 0)
> +        return NS_ERROR_FAILURE;
> +
> +    MOZ_ASSERT((code & TranscodeResult_Error) != 0);
> +    JS_ClearPendingException(cx);

This hunk seems to change existing behavior: an error value is returned with no exception pending whereas before there would be an exception pending.  Probably best to have an xpconnect peer review this code if this change was intentional.

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8770662 [details] [diff] [review]
Distinguish failure reasons of JS_{En,De}codeScript.

Mwu, do you happen to know if the JSContext error provided by XDR matters to any Xul components, or for evalInSandbox function, as mentioned in Bug 648125?

Also, Would you know who might be the best person to ask for future XDR-related reviews in XPConnect?

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8770662 [details] [diff] [review]
Distinguish failure reasons of JS_{En,De}codeScript.

(oops, r+ with comments addressed)

Comment 7

[Michael Wu [:mwu]]

Comment on attachment 8770662 [details] [diff] [review]
Distinguish failure reasons of JS_{En,De}codeScript.

Last time I checked, users of XDR encoding/decoding don't care about what the error actually is and just fall back to parsing the JS normally. We had a space saving hack that could also strip out the original JS, but I don't think it landed.

Clearing review since I haven't actually reviewed the patch - don't remember the code anymore.

Don't know who'd be the right person for XDR/XPConnect reviews now.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/539b9c11ee64
Distinguish failure reasons of JS_{En,De}codeScript. r=luke

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for xpcshell crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=32229723&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/62a913298af6

Comment 10

[Nicolas B. Pierron [:nbp]]

(In reply to Wes Kocher (:KWierso) from comment #9)
> I had to back this out for xpcshell crashes like
> https://treeherder.mozilla.org/logviewer.html#?job_id=32229723&repo=mozilla-
> inbound
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/62a913298af6

Ok, the problem seems to be minor issue, I added a reset of the out-param next to the call to postProcessContextErrors in codeScript / codeFunction functions, such that we do not leave a dangling pointer to half-initialized structures when we return from these functions.

I will re-land with this fix.

Comment 11

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #10)
> I will re-land with this fix.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b2fbc4d45e6916e04db9678adcc7ce27da3f669

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c72a107222a
Distinguish failure reasons of JS_{En,De}codeScript. r=luke

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8c72a107222a