Closed Bug 893679 Opened 7 years ago Closed 7 years ago

Crash at weird memory address with testcase involving ParallelArray

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox24 --- unaffected
firefox25 --- fixed
firefox-esr17 --- unaffected
firefox-esr24 --- unaffected
b2g18 --- unaffected

People

(Reporter: gkw, Assigned: shu)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, testcase, Whiteboard: [jsbugmon:])

Attachments

(3 files, 1 obsolete file)

Attached file stack
s = newGlobal();
function ff(code) {
    try {
        evalcx(code, s)
    } catch (e) {}
}
ff("\
    Object.defineProperty(this,\"v2\",{\
        get:function(){\
            f1({})\
        }\
    });\
    o2 = new Object;\
    f2 = (function(j){\
        if(j) {}\
    });\
    f1 = f2;\
    for(v of v2) {}\
");
ff("\
    ParallelArray([89], function(x0){\
        if(x0%86==71) {\
            f1(o2.p)\
        }\
    })\
")

crashes js opt shell (threadsafe, deterministic) on m-c changeset 18467a85acf6 with --baseline-eager at a weird memory address.

Not sure what's going on here, setting s-s just-in-case.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
I can reproduce, looking at it.
Attached patch fix (obsolete) — Splinter Review
Assignee: general → shu
Attachment #775653 - Flags: review?(bhackett1024)
Comment on attachment 775653 [details] [diff] [review]
fix

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

This doesn't seem like the right fix.  property->empty() means that no object with that type has that property, but even if !property->empty() there could be some objects with the type which have the property, and which could generate an undefined value when read from which would normally be reflected via a Monitor call.

I think that, as long as you're not able to call TypeScript::Monitor from within the ParallelGetPropertyIC VM call, you always need a barrier when generating ParallelGetPropertyIC instructions.
Attachment #775653 - Flags: review?(bhackett1024)
Oh, that should read 'there could be some objects with the type which *don't* have the property'
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
Attached patch fix v2Splinter Review
Yes, you were right about the previous patch, that fix was bogus.
Attachment #775653 - Attachment is obsolete: true
Attachment #775710 - Flags: review?(bhackett1024)
Comment on attachment 775710 [details] [diff] [review]
fix v2

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

::: js/src/ion/IonBuilder.cpp
@@ +7956,5 @@
> +            barrier = true;
> +        break;
> +      default:
> +        MOZ_ASSUME_UNREACHABLE("No such execution mode");
> +    }

This code is pretty verbose.  How about:

if (info().executionMode() == ParallelExecution &&
    !types->hasType(types::Type::UndefinedType()))
{
    barrier = true;
}

(The NULL check is not necessary on |types|)
Attachment #775710 - Flags: review?(bhackett1024) → review+
(In reply to Brian Hackett (:bhackett) from comment #7)
> Comment on attachment 775710 [details] [diff] [review]
> fix v2
> 
> Review of attachment 775710 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/IonBuilder.cpp
> @@ +7956,5 @@
> > +            barrier = true;
> > +        break;
> > +      default:
> > +        MOZ_ASSUME_UNREACHABLE("No such execution mode");
> > +    }
> 
> This code is pretty verbose.  How about:
> 
> if (info().executionMode() == ParallelExecution &&
>     !types->hasType(types::Type::UndefinedType()))
> {
>     barrier = true;
> }
> 
> (The NULL check is not necessary on |types|)

Fine with me. The verbosity was to keeping with other sites where we case on execution mode, to be extra explicit about what we're doing.
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/0c576cf51a80
user:        Shu-yu Guo
date:        Wed Jul 03 09:47:28 2013 -0700
summary:     Bug 886102 - Ignore idempotency for parallel ICs. (r=nmatsakis)
Blocks: 886102
r=bhackett or irc.
Attachment #776037 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2a9ef042a6d0
https://hg.mozilla.org/mozilla-central/rev/6adcce906728
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Group: core-security
You need to log in before you can comment on or make changes to this bug.