905903 - Assertion failure: [barrier verifier] Unmarked edge: baseline-monitor-stub-ioncode, at gc/Verifier.cpp:570 with setObjectMetadataCallback

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision a8daa428ccbc (run with --fuzzing-safe):


var lfcode = new Array();
lfcode.push("");
lfcode.push("");
lfcode.push("1");
lfcode.push("");
lfcode.push("");
lfcode.push("");
lfcode.push("");
lfcode.push("2");
lfcode.push("gczeal(4);");
lfcode.push("setObjectMetadataCallback(function(obj) {});");
while (true) {
  var file = lfcode.shift(); if (file == undefined) { break; }
  loadFile(file)
}
function loadFile(lfVarx) {
    try {
        if (lfVarx.substr(-3) != ".js" && lfVarx.length != 1) {
            switch (lfRunTypeId) {
                case 2: new Function(lfVarx)(); break;
            }
        } else if (!isNaN(lfVarx)) {
            lfRunTypeId = parseInt(lfVarx);
        }
    } catch (lfVare) {
    }
}

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

Again like bug 886850, same signature. Needinfo from Brian.

Comment 3

[Christian Holler (:decoder)]

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

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/be1399f8f973
user:        Brian Hackett
date:        Thu May 30 17:37:22 2013 -0600
summary:     Bug 850026 - Allow metadata objects to be associated with JS objects, and add a hook for attaching metadata to newly created objects, r=luke.

This iteration took 1.682 seconds to run.

Comment 4

[Andrew McCreight [:mccr8]]

Could you maybe look at this, djvj?  It looks like it involves baseline.  Thanks.

Comment 5

[Kannan Vijayan [:djvj]]

Ok.

Comment 6

[Andrew McCreight [:mccr8]]

I'm going to mark this sec-high until we know that this is just some weirdness of setObjectMetadataCallback in particular.

Comment 7

[Christian Holler (:decoder)]

Putting needinfo again on djvj. This keeps filling up my inbox day by day, it produces lots of signatures.

Comment 8

[Kannan Vijayan [:djvj]]

This seems to be the issue:

#0  js::jit::BaselineScript::purgeOptimizedStubs (this=0x95c5b68, zone=0x95cb248)
    at /home/kvijayan/Checkouts/mozilla-inbound/js/src/jit/BaselineJIT.cpp:796
#1  0x084f8882 in js::jit::FinishDiscardBaselineScript (fop=0x95942b0, script=0xf6f2b100)
    at /home/kvijayan/Checkouts/mozilla-inbound/js/src/jit/BaselineJIT.cpp:862
#2  0x08343c2c in js::ReleaseAllJITCode (fop=0x95942b0)
    at /home/kvijayan/Checkouts/mozilla-inbound/js/src/jsgc.cpp:4979
#3  0x083290b3 in js::SetObjectMetadataCallback (cx=0x95a6938, callback=
    0x821af28 <ShellObjectMetadataCallback(JSContext*, JSObject**)>)

It happens right before the verifier error.  I suspect that purgeOptimizedStubs doesn't properly trace monitored stub chains.  It may have something to do with the fact that purgeOptimizedStubs is called from a non-GC context.

Comment 9

[Kannan Vijayan [:djvj]]

Attachment: pre-barrier-fix.patch

Found it.  When clearing out the type monitored stubs, we clear out the optimized stubs, but we only actually trace the fallback stub.  This makes no sense since the fallback stub is the one that survives the clearing out, and the optimized stubs are the ones that don't.

Comment 10

[Terrence Cole [:terrence]]

Comment on attachment 808234 [details] [diff] [review]
pre-barrier-fix.patch

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

Sorry for the delay, but I wanted to make sure I understood what's going on here.

r=me, Good find!

::: js/src/jit/BaselineIC.cpp
@@ +466,5 @@
>  {
>      if (zone->needsBarrier()) {
>          // We are removing edges from monitored stubs to gcthings (IonCode).
>          // Perform one final trace of all monitor stubs for incremental GC,
>          // as it must know about those edges.

Could you update this comment to clarify what you explained in comment 9 of this bug?

Comment 11

[Kannan Vijayan [:djvj]]

(In reply to Terrence Cole [:terrence] from comment #10)
> ::: js/src/jit/BaselineIC.cpp
> @@ +466,5 @@
> >  {
> >      if (zone->needsBarrier()) {
> >          // We are removing edges from monitored stubs to gcthings (IonCode).
> >          // Perform one final trace of all monitor stubs for incremental GC,
> >          // as it must know about those edges.
> 
> Could you update this comment to clarify what you explained in comment 9 of
> this bug?

The comment actually correctly describes the new behaviour (we trace the monitor stubs).  The problem was that the code prior to this patch wasn't actually implementing that behaviour, instead it was tracing just the fallback monitor stub.

Comment 12

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbecef67074c

Comment 13

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

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

Comment 14

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 16

[David Chan [:dchan]]

Cleaning up list of security bugs for b2g18. This bug doesn't need to be backported either due to it affecting a later version of Fx or another reason.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

can we get a beta uplift nomination here?  if this is low risk to land on beta we should get it in.

Comment 18

[Kannan Vijayan [:djvj]]

Comment on attachment 808234 [details] [diff] [review]
pre-barrier-fix.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
N/A.  Part of some core baseline code before it landed.

User impact if declined: 
Use-after-free caused by incomplete tracing of objects.

Testing completed (on m-c, etc.): 
Green in central for over a month.

Risk to taking this patch (and alternatives if risky):
Very low.
 
String or IDL/UUID changes made by this patch:
N/A

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8f44e7a03c4c

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/8f44e7a03c4c

Comment 21

[Alex Keybl [:akeybl]]

ESR24 patch needed - we'd like to go to build for ESR late Tuesday or Wednesday.

Comment 22

[Kannan Vijayan [:djvj]]

Attachment: pre-barrier-fix-esr24.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Sec-high bug.

User impact if declined: 
Garbage collection bug in tracing baseline ICs.  Crashes.  Use-after-free.

Fix Landed on Version:
Firefox 29

Risk to taking this patch (and alternatives if risky):
Very low risk. 

String or UUID changes made by this patch: 
N/A

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

ESR234 version

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/eb5cfa4a85a4