1419094 - Result-ify XDR transcoding

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Ted Campbell [:tcampbell]]

While looking at Bug 1390856 and other XDR crashes, I threaded a Result type into the XDR internals to better handle errors. This is focused on internal methods and anything that may XDRState::fail.

We can decide if this is a good approach or not, but for now it identifies potential problems with XDR. I'd like to defer landing this for a bit anyways until we are happier about XDR crash numbers so we uplifts are easier.

Comment 1

[Ted Campbell [:tcampbell]]

Attachment: 0001-Bug-1419094-Clean-up-XDR-error-handling.patch

Make use of ScopeExit instead of manual handling on errors

Comment 2

[Ted Campbell [:tcampbell]]

Attachment: 0002-Bug-1419094-Result-ify-XDR-internal-code.patch

Equivalent transform with XdrResult being a simple struct around a bool. We should look at using a standard mozilla::Result instead, but that should be a small follow-up patch.

Comment 3

[Ted Campbell [:tcampbell]]

Attachment: 0003-Bug-1419094-Remove-XDRState-codeConstValue.patch

Right now XDRState directly has low level int/string serialization APIS and top-level codeScript/codeFunction APIs. Remove codeConstValue since it is odd one out and we are a little inconsistent about handling.

Comment 4

[Ted Campbell [:tcampbell]]

There are currently 35 locations where we return XdrResult::fail() (previously false), without calling XDRState::fail(). Almost all are OOM. These get caught by postProcessContextErrors (See Bug 1390856)

Comment 5

[Ted Campbell [:tcampbell]]

Attachment: 0002-Bug-1419094-Result-ify-XDR-internal-code.patch

Remove junk file from patch.

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8930142 [details] [diff] [review]
0002-Bug-1419094-Result-ify-XDR-internal-code.patch

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

::: js/src/vm/Xdr.h
@@ +28,5 @@
> +// Result type for XDR internal functions. Functions that may call
> +// XDRState::fail(...) should return this type.
> +struct XdrResult
> +{
> +    bool result;

this would be even better if this would be the enumerated type instead of a boolean.
Such that failures would be:

  return Err(JS::TranscodeResult_Throw);

instead of:

  return xdr->fail(JS::TranscodeResult_Throw)

@@ +39,5 @@
> +    static MOZ_ALWAYS_INLINE XdrResult fail() { return { false }; };
> +};
> +
> +// A try/result macro for XDR. See MOZ_TRY for more information.
> +#define XDR_TRY(expr) \

use MOZ_TRY and mozilla::Result or JS::Result.

@@ +510,5 @@
>      void endSubTree() override;
>  
>      // Append the content collected during the incremental encoding into the
>      // buffer given as argument.
> +    MOZ_MUST_USE js::XdrResult linearize(JS::TranscodeBuffer& buffer);

with mozilla::Result there would no longer be any need for MOZ_MUST_USE as this is part of the Result class annotation.

Comment 7

[Nicolas B. Pierron [:nbp]]

Attachment: Result-ify XDR functions.

Delta:
 - Add XDRResult as an alias to Result<Ok, JS::TranscodeResult>
 - Move resultCode_ into XDRCoderBase, to be used in AutoXDRTree destructor.
 - Make resultCode_ debug-only, and rely exclusively on the returned value.
 - Make XDRState<mode>::fail(...) checks that only if an exception is reported
   then we have the JS::TranscodeResult_Throw code.
 - Within XDR functions:
   . return true; --> return Ok();
   . return false --> return xdr->fail(JS::TranscodeResult_Throw);
   . if (!XDR(...)) return false; --> MOZ_TRY(XDR(...));
   . if (!XDR(...)) { ...; return false; }
       -->
     auto guard = mozilla::MakeScopeExit([&] { ...; });
     MOZ_TRY(XDR(...));
     guard.release();
 - Move the isAligned resultCode() condition to AutoXDRTree::~AutoXDRTree
   MOZ_ASSERT.

This patch fixes the issue found in Bug 1401939, which caused the patch to be
backed out, by properly setting the resultCode_ to JS::TranscodeResult_Throw
when an allocation failure happen.

This patch is based on Bug 1401939 changes, and should be landed with Bug 1401939 patch.
(Sorry this is a big review)

Comment 8

[Ted Campbell [:tcampbell]]

Comment on attachment 8958507 [details] [diff] [review]
Result-ify XDR functions.

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

Yay! Thanks for getting this done.

::: js/src/jsapi.cpp
@@ +7631,5 @@
>  {
>      XDREncoder encoder(cx, buffer, buffer.length());
>      RootedFunction funobj(cx, &funobjArg->as<JSFunction>());
> +    XDRResult res = encoder.codeFunction(&funobj);
> +    if (res.isErr()) {

Much nicer :)

::: js/src/vm/JSScript.cpp
@@ +2150,4 @@
>  
>          size_t byteLen = compressedLength ? compressedLength : (len * sizeof(char16_t));
>          if (mode == XDR_DECODE) {
>              uint8_t* p = xdr->cx()->template pod_malloc<uint8_t>(Max<size_t>(byteLen, 1));

mozilla::UniquePtr<uint8_t[], JS::FreePolicy> p(
    xdr->cx()->template pod_malloc<uint8_t>(Max<size_t>(byteLen, 1)));

::: js/src/vm/Xdr.h
@@ +184,5 @@
>  
>  class XDRCoderBase
>  {
> +#ifdef DEBUG
> +  private:

Nit: put private outside the macro

Comment 9

[Ted Campbell [:tcampbell]]

Comment on attachment 8930142 [details] [diff] [review]
0002-Bug-1419094-Result-ify-XDR-internal-code.patch

We are taking :nbp's version of this patch instead.

Comment 10

[Ted Campbell [:tcampbell]]

Leave-open. Once :nbp's patch goes in, I'll rebase the other cleanups on top.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/43dff1123cfe
Result-ify XDR functions. r=tcampbell

Comment 12

[Ted Campbell [:tcampbell]]

Attachment: Part-2-Cleanup-some-XDR-apis.patch

These are some small cleanups I wanted while we are touching this stuff.

Comment 13

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8958563 [details] [diff] [review]
Part-2-Cleanup-some-XDR-apis.patch

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

Way better to have all these XDR types in the same header!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/384537aaeeb8
Cleanup some XDR apis. r=nbp

Comment 15

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/43dff1123cfe

Comment 16

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/384537aaeeb8