Distinguish XDR error code.

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 affected, firefox52 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
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.
Attachment #8769802 - Flags: review?(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.
Attachment #8769802 - Attachment is obsolete: true
Attachment #8769802 - Flags: review?(luke)
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.
Attachment #8770662 - Flags: review?(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 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?
Attachment #8770662 - Flags: review?(mwu.code)
Comment on attachment 8770662 [details] [diff] [review]
Distinguish failure reasons of JS_{En,De}codeScript.

(oops, r+ with comments addressed)
Attachment #8770662 - Flags: review?(luke) → review+
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.
Attachment #8770662 - Flags: review?(mwu.code)
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/539b9c11ee64
Distinguish failure reasons of JS_{En,De}codeScript. r=luke
(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.
(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
Flags: needinfo?(nicolas.b.pierron)
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c72a107222a
Distinguish failure reasons of JS_{En,De}codeScript. r=luke
https://hg.mozilla.org/mozilla-central/rev/8c72a107222a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1347120
You need to log in before you can comment on or make changes to this bug.