Summary: | JSGlobalObject::fireWatchpointAndMakeAllArrayStructuresSlowPut() should fire its watchpoint as the last step. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | ews-watchlist, fpizlo, keith_miller, msaboff, rmorisset, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Mark Lam
2019-11-05 15:05:14 PST
Created attachment 382858 [details]
proposed patch.
Comment on attachment 382858 [details]
proposed patch.
r=me
Comment on attachment 382858 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=382858&action=review r=me too > Source/JavaScriptCore/ChangeLog:11 > + JSGlobalObject::fireWatchpointAndMakeAllArrayStructuresSlowPut() should make all > + the array structures SlowPut before firing the watchpoint. Otherwise, the > + concurrent JIT may think it's grabbing the slow put version of the structure, but > + is actually grabbing the non-SlowPut version because it happened to beat the > + mutator in a race to read the structure before the mutator makes it SlowPut. put below "Reviewed by" Comment on attachment 382858 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=382858&action=review >> Source/JavaScriptCore/ChangeLog:11 >> + mutator in a race to read the structure before the mutator makes it SlowPut. > > put below "Reviewed by" Thanks for catch that. Created attachment 382881 [details]
patch for landing.
Comment on attachment 382881 [details] patch for landing. Attachment 382881 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/13217556 New failing tests: stress/toctou-having-a-bad-time-new-array.js.dfg-eager stress/toctou-having-a-bad-time-new-array.js.ftl-eager Created attachment 382905 [details]
proposed patch.
I added more code to placate a DFG_ASSERT in SpeculativeJIT::compileNewArray(). We could just remove that assert, but I felt that there's some value in keeping it. So, let's get another review to see if anyone is strongly opposed to this change.
Comment on attachment 382905 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=382905&action=review > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7987 > + sleep(Seconds::fromMicroseconds(100)); This is crazy. We can’t have code in the compiler like this loop. We should restructure the code to not have this problem. Created attachment 382933 [details]
proposed patch.
Comment on attachment 382933 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=382933&action=review > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8004 > + if (!globalObject->hasStartedTransitioningToHavingABadTime() && !isHavingABadTime && !hasAnyArrayStorage(node->indexingType())) { this is super localized to this one place (there is probably other code you're not catching). It's also not really needed. I'm not a fan of adding more code to appease an assertion like this with a huge comment. It seems better to just remove the assertion if that's valid. (Also note: the FTL has no such assertion.) It's not worth bending over backwards for an assertion we know is ultimately wrong. Created attachment 382977 [details]
proposed patch.
Comment on attachment 382977 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=382977&action=review > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1584 > + // Make sure that all allocations or indexed storage transitions that are inlining > + // the assumption that it's safe to transition to a non-SlowPut array storage don't > + // do so anymore. maybe also worth a note on the store ordering here? Thanks for the review. (In reply to Saam Barati from comment #12) > Comment on attachment 382977 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382977&action=review > > > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1584 > > + // Make sure that all allocations or indexed storage transitions that are inlining > > + // the assumption that it's safe to transition to a non-SlowPut array storage don't > > + // do so anymore. > > maybe also worth a note on the store ordering here? Will do. Landed in r252160: <http://trac.webkit.org/r252160>. |