1370869 - XDR decoding creates scope Data objects and then deletes them

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

Scope::XDRSizedBindingNames creates and returns a scope Data object of the required length.  In most cases we then check if the length is zero and if so delete it again.

Not only is this wasteful, but it bumps into one of the GC's rough edges: we can't immediately delete objects that contain GCPtrs because the store buffer may contain pointers into them.  Instead we have to queue them for deletion at a safe time (hopefully we can kill this with bug 1370868 but that's not certain).  This deferred deletion was only intended to be used to handle errors (e.g. OOM initialising some object), not normal code paths that happen regularly.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1370869-xdr-size-bindings

Change Scope::XDRSizedBindingNames to only allocate the Scope::Data if length != 0 and also return the length.

Comment 2

[Shu-yu Guo [:shu]]

(In reply to Jon Coppeard (:jonco) from comment #0)
> Scope::XDRSizedBindingNames creates and returns a scope Data object of the
> required length.  In most cases we then check if the length is zero and if
> so delete it again.
> 
> Not only is this wasteful, but it bumps into one of the GC's rough edges: we
> can't immediately delete objects that contain GCPtrs because the store
> buffer may contain pointers into them.  Instead we have to queue them for
> deletion at a safe time (hopefully we can kill this with bug 1370868 but
> that's not certain).  This deferred deletion was only intended to be used to
> handle errors (e.g. OOM initialising some object), not normal code paths
> that happen regularly.

Interesting, did this show up in profiles?

Comment 3

[Shu-yu Guo [:shu]]

Comment on attachment 8875315 [details] [diff] [review]
bug1370869-xdr-size-bindings

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

Thanks for the patch.

::: js/src/vm/Scope.cpp
@@ +588,5 @@
>              return false;
>  
>          if (mode == XDR_DECODE) {
> +            if (length == 0)
> +                return false;

Should be clearer and equivalent to check if (!data).

@@ +790,5 @@
> +                data->nonPositionalFormalStart = nonPositionalFormalStart;
> +                data->varStart = varStart;
> +            } else {
> +                MOZ_ASSERT(!nonPositionalFormalStart);
> +                MOZ_ASSERT(!varStart);

What happened to the !data->nextFrameSlot assert?

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Shu-yu Guo [:shu] from comment #3)
> What happened to the !data->nextFrameSlot assert?

I deleted it because |data| is null at this point.  I'll add an assert later on that nextFrameSlot ends up as zero if there are no bindings.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a43148a783b
Don't allocate scope bindings data only to delete it immediately r=shu

Comment 6

[Carsten Book [:Tomcat]]

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

Comment 7

[Jon Coppeard (:jonco)]

(In reply to Shu-yu Guo [:shu] from comment #2)
> Interesting, did this show up in profiles?

This showed up trying to land bug 1358073 on esr52.  We fall foul of bug 1371234.  The cleanup mechanism for GC managed objects destroyed outside of the GC was not intended to be used as part of normal operation but as a way of handling e.g. OOM during initialisation.

Comment 8

[Jon Coppeard (:jonco)]

Looking at the code some more I realised that in the non-zero-length case XDR decoding still creates scope data, copies it and then deletes the original.

Comment 9

[Jon Coppeard (:jonco)]

Attachment: bug1370869-dont-copy-scope-data-for-xdr

This patch refactors scope creation so that it doesn't copy the scope data for the XDR decode case.

This doesn't set nextFrameSlot on the data passed in by the frontend any more (it just sets it on the copy) but as far as I can see that is not used.

This patch applies over a backout of the previous patch.

Comment 10

[Shu-yu Guo [:shu]]

Comment on attachment 8876071 [details] [diff] [review]
bug1370869-dont-copy-scope-data-for-xdr

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

::: js/src/vm/Scope.cpp
@@ +519,5 @@
>                       uint32_t firstFrameSlot, HandleScope enclosing)
>  {
> +    MOZ_ASSERT(data, "LexicalScopes should not be created if there are no bindings.");
> +
> +    // The data that's passed in may be from the frontend and LifoAlloc'd.

Please update this comment and others like it below. The always-copy behavior was for uniformity of the code, thus the "may be from the frontend". Now the API is that |create| always copies the data, and |createWithData| mutates then moves the data, this should say the data always comes from the frontend.

@@ +762,5 @@
>  
>      {
> +        Maybe<Rooted<UniquePtr<Data>>> uniqueData;
> +        if (mode == XDR_DECODE)
> +            uniqueData.emplace(cx, data);

Nice, removed the ScopeExit.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d54918df1a29
Don't copy scope data in XDR decode r=shu

Comment 12

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

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

Comment 13

[Ekanan Ketunuti]

> This patch applies over a backout of the previous patch.

I got a bit confused with the milestone of mozilla-central changeset d54918df1a29 and the tracking flags here.
(milestone says 55.0a1, tracking flag says 56)

I think d54918df1a29 just missed the train. Should d54918df1a29 be uplift to 55 branch as well?

Comment 14

[Jon Coppeard (:jonco)]

Attachment: bug1370869-beta-55

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1358073.
[User impact if declined]: Possible OOM crashes XDR decoding when the nursery is full.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This doesn't affect behaviour, it's a refactoring that reduces the number of memory allocations when XDR decoding.
[String changes made/needed]: None.

The first patch for this has landed on 55 already.

Comment 15

[Julien Cristau [:jcristau] (back April 22)]

This was reopened after landing the first patch so resetting 55 status (which only has that first patch) to affected.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8877528 [details] [diff] [review]
bug1370869-beta-55

Looks like a big refactor, but it sounds like we should take this for 55 or else back out the other huge patch.  Since we're still early in beta I'd rather uplift part 2.

Comment 17

[Ekanan Ketunuti]

NI ryanvm just in case sheriffs missed uplift tracking due to TM being set as 55.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Not necessary (we go off tracking flags, not TM), but thanks anyway :)

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Jon pushed this:
https://hg.mozilla.org/releases/mozilla-beta/rev/ea84df3bdb99

Comment 20

[Ryan VanderMeulen [:RyanVM]]

I think we need an ESR52 patch here as well?

Comment 21

[Jon Coppeard (:jonco)]

Attachment: bug1370869-52

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Required for backport of patch in bug 1358073.
User impact if declined: Bug 1358073 fixes some crashes / possible vulnerability.
Fix Landed on Version: 56
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

This patch applies on top of the bug1358073-esr52 patch in the other bug.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/3b8fde840188