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 beat the mutator in a race to read the structure before the mutator makes it SlowPut. <rdar://problem/56813514>
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>.