Closed
Bug 893679
Opened 11 years ago
Closed 11 years ago
Crash at weird memory address with testcase involving ParallelArray
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla25
Tracking | Status | |
---|---|---|
firefox24 | --- | unaffected |
firefox25 | --- | fixed |
firefox-esr17 | --- | unaffected |
firefox-esr24 | --- | unaffected |
b2g18 | --- | unaffected |
People
(Reporter: gkw, Assigned: shu)
References
Details
(Keywords: crash, regression, testcase, Whiteboard: [jsbugmon:])
Attachments
(3 files, 1 obsolete file)
1.41 KB,
text/plain
|
Details | |
1.25 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
2.07 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•11 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
Comment 1•11 years ago
|
||
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Assignee | ||
Comment 2•11 years ago
|
||
I can reproduce, looking at it.
Assignee | ||
Comment 3•11 years ago
|
||
Assignee: general → shu
Attachment #775653 -
Flags: review?(bhackett1024)
Comment 4•11 years ago
|
||
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)
Comment 5•11 years ago
|
||
Oh, that should read 'there could be some objects with the type which *don't* have the property'
Updated•11 years ago
|
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
Assignee | ||
Comment 6•11 years ago
|
||
Yes, you were right about the previous patch, that fix was bogus.
Attachment #775653 -
Attachment is obsolete: true
Attachment #775710 -
Flags: review?(bhackett1024)
Comment 7•11 years ago
|
||
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+
Assignee | ||
Comment 8•11 years ago
|
||
(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.
Reporter | ||
Comment 9•11 years ago
|
||
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
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2a9ef042a6d0 https://hg.mozilla.org/mozilla-central/rev/6adcce906728
Status: NEW → RESOLVED
Closed: 11 years ago
status-firefox25:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Updated•11 years ago
|
status-firefox-esr17:
--- → unaffected
status-firefox-esr24:
--- → unaffected
Updated•11 years ago
|
status-firefox24:
--- → unaffected
Updated•11 years ago
|
status-b2g18:
--- → unaffected
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•