Bug 203867 - JSGlobalObject::fireWatchpointAndMakeAllArrayStructuresSlowPut() should fire its watchpoint as the last step.
Summary: JSGlobalObject::fireWatchpointAndMakeAllArrayStructuresSlowPut() should fire ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-05 15:05 PST by Mark Lam
Modified: 2019-11-06 16:29 PST (History)
9 users (show)

See Also:


Attachments
proposed patch. (4.22 KB, patch)
2019-11-05 16:13 PST, Mark Lam
rmorisset: review+
Details | Formatted Diff | Diff
patch for landing. (4.28 KB, patch)
2019-11-05 18:20 PST, Mark Lam
ews: commit-queue-
Details | Formatted Diff | Diff
proposed patch. (10.30 KB, patch)
2019-11-06 01:30 PST, Mark Lam
sbarati: review-
Details | Formatted Diff | Diff
proposed patch. (10.66 KB, patch)
2019-11-06 09:55 PST, Mark Lam
sbarati: review-
Details | Formatted Diff | Diff
proposed patch. (7.04 KB, patch)
2019-11-06 16:13 PST, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2019-11-05 15:05:14 PST
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>
Comment 1 Mark Lam 2019-11-05 16:13:09 PST
Created attachment 382858 [details]
proposed patch.
Comment 2 Robin Morisset 2019-11-05 16:15:44 PST
Comment on attachment 382858 [details]
proposed patch.

r=me
Comment 3 Saam Barati 2019-11-05 17:16:57 PST
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 4 Mark Lam 2019-11-05 17:27:46 PST
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.
Comment 5 Mark Lam 2019-11-05 18:20:01 PST
Created attachment 382881 [details]
patch for landing.
Comment 6 Build Bot 2019-11-05 21:11:09 PST
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
Comment 7 Mark Lam 2019-11-06 01:30:53 PST
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 8 Saam Barati 2019-11-06 07:41:10 PST
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.
Comment 9 Mark Lam 2019-11-06 09:55:33 PST
Created attachment 382933 [details]
proposed patch.
Comment 10 Saam Barati 2019-11-06 11:53:38 PST
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.
Comment 11 Mark Lam 2019-11-06 16:13:28 PST
Created attachment 382977 [details]
proposed patch.
Comment 12 Saam Barati 2019-11-06 16:15:21 PST
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?
Comment 13 Mark Lam 2019-11-06 16:28:15 PST
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.
Comment 14 Mark Lam 2019-11-06 16:29:57 PST
Landed in r252160: <http://trac.webkit.org/r252160>.