857094 - Assertion failure: [barrier verifier] Unmarked edge: objectElements, at gc/Verifier.cpp:596

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision aae004a3c5d9 (run with --ion-eager):


gczeal(4);
var shape = [5];
for (var i = 0; i < 7; i++) {
  shape.push(i+1);
  var p = new ParallelArray(shape, function(k) { return k; });
  var r = p.scatter([0,1,0,3,04 ], 9, function (a,b) { return a+b; }, 10);
}

Comment 1

[Christian Holler (:decoder)]

Marked s-s because this involves GC.

Comment 2

[Shu-yu Guo [:shu]]

I cannot reproduce locally with a debug shell and --ion-eager.

Comment 3

[Christian Holler (:decoder)]

(In reply to Shu-yu Guo [:shu] from comment #2)
> I cannot reproduce locally with a debug shell and --ion-eager.

What OS are you using, and what build options? The debug shell was built with --enable-debug --enable-optimize --enable-valgrind on Linux, built for x86 (32 bit).

If that doesn't help reproducing, let me know.

Comment 4

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   125555:b00eb1ef1517
user:        Nicholas D. Matsakis
date:        Tue Mar 19 22:12:27 2013 -0400
summary:     Bug 829602 - Enable self-hosted parallelarray r=dvander,till

This iteration took 170.963 seconds to run.

Comment 5

[Shu-yu Guo [:shu]]

(In reply to Christian Holler (:decoder) from comment #3)
> (In reply to Shu-yu Guo [:shu] from comment #2)
> > I cannot reproduce locally with a debug shell and --ion-eager.
> 
> What OS are you using, and what build options? The debug shell was built
> with --enable-debug --enable-optimize --enable-valgrind on Linux, built for
> x86 (32 bit).
> 
> If that doesn't help reproducing, let me know.

Interesting. This is only reproducible with a non-threadsafe shell for me.

Comment 6

[Shu-yu Guo [:shu]]

Attachment: fix

Missed checking if MStoreElement needs a prebarrier when inlining UnsafeSetElement.

Comment 7

[Sean Stangl [:sstangl]]

Comment on attachment 733145 [details] [diff] [review]
fix

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

::: js/src/ion/MCallOptimize.cpp
@@ +1000,5 @@
>                                                callInfo.getArg(elemi),
>                                                /* needsHoleCheck = */ false);
>      store->setRacy();
>  
> +    // Determine whether a write barrier is required.

The code following the comment is nicely self-explanatory, so we can remove the comment. (jsop_setelem_dense() has a similar redundant comment which can also be removed.)

Comment 8

[Shu-yu Guo [:shu]]

(In reply to Sean Stangl [:sstangl] from comment #7)
> Comment on attachment 733145 [details] [diff] [review]
> fix
> 
> Review of attachment 733145 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/MCallOptimize.cpp
> @@ +1000,5 @@
> >                                                callInfo.getArg(elemi),
> >                                                /* needsHoleCheck = */ false);
> >      store->setRacy();
> >  
> > +    // Determine whether a write barrier is required.
> 
> The code following the comment is nicely self-explanatory, so we can remove
> the comment. (jsop_setelem_dense() has a similar redundant comment which can
> also be removed.)

I thought it might be good to have it in there since 'barrier' is overloaded between type and write barriers. But perhaps the absence of a TypeSet around the code is enough to distinguish it as a write vs type barrier when eyeballing.

Comment 9

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1aa1dc84a1a4

try: https://tbpl.mozilla.org/?tree=Try&rev=21c066c8cc1e

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1aa1dc84a1a4

Comment 11

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 12

[Daniel Veditz [:dveditz]]

Since this affected previous branches at the time it landed (fx22) and is a security bug of unstated severity this patch should have sought sec-approval before landing.

Please put together a Fx22 patch and request aurora approval, or explain that this turns out not to have been a security bug after all.

Comment 13

[Shu-yu Guo [:shu]]

Attachment: patch for Firefox 22

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 807853
User impact if declined: Possibly exploitable GC security bug
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): None
String or IDL/UUID changes made by this patch:

This is the commmit from m-c, should apply cleanly to Aurora.

Comment 14

[Alex Keybl [:akeybl]]

Was this caused by bug 829602 (FF22) or bug 807853 (FF21)?

Comment 15

[Shu-yu Guo [:shu]]

(In reply to Alex Keybl [:akeybl] from comment #14)
> Was this caused by bug 829602 (FF22) or bug 807853 (FF21)?

Sorry, it's 829602. The code was added in 807853, but it had no users until 829602 landed.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7057622584a0