Bug 203867 – JSGlobalObject::fireWatchpointAndMakeAllArrayStructuresSlowPut() should fire its watchpoint as the last step.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

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>

[Mark Lam]

Created attachment 382858 [details]
proposed patch.

[Robin Morisset]

Comment on attachment 382858 [details]
proposed patch.

r=me

[Saam Barati]

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"

[Mark Lam]

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.

[Mark Lam]

Created attachment 382881 [details]
patch for landing.

[EWS Watchlist]

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

[Mark Lam]

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.

[Saam Barati]

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.

[Mark Lam]

Created attachment 382933 [details]
proposed patch.

[Saam Barati]

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.

[Mark Lam]

Created attachment 382977 [details]
proposed patch.

[Saam Barati]

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?

[Mark Lam]

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.

[Mark Lam]

Landed in r252160: <http://trac.webkit.org/r252160>.