Bug 184773 – In FTLLowerDFGToB3.cpp::compileCreateRest, always use a contiguous array as the indexing type when under isWatchingHavingABadTimeWatchpoint

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Robin Morisset]

Currently, we call restParameterStructure(), which in turn returns arrayStructureForIndexingTypeDuringAllocation(ArrayWithContiguous).
arrayStructureForIndexingTypeDuringAllocation uses m_arrayStructureForIndexingShapeDuringAllocation, which is set to SlowPutArrayStorage when we are 'having a bad time'.
This is problematic, because the structure is then passed to allocateUninitializedContiguousJSArray, which ASSERTs that the indexing type is contiguous (or int32).

We solve the problem by using originalArrayStructureForIndexingType which always returns a structure with the right indexing type (contiguous), even if we are having a bad time.
This is safe, as we are under isWatchingHavingABadTimeWatchpoint, so if we have a bad time, the code we generate will never be installed.

<rdar://problem/37773612>

[Robin Morisset]

Created attachment 338320 [details]
Patch

[Filip Pizlo]

Comment on attachment 338320 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=338320&action=review

R=me with that change.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5284
> +            RegisteredStructure structure = m_graph.registerStructure(globalObject->originalArrayStructureForIndexingType(ArrayWithContiguous));

I think it would be better if you added a JSGlobalObject helper method called originalRestParameterStructure. I think we have the restParameterStructure method to abstract the fact that it’s just the contiguous array structure.  It would be good if you also had such an abstraction for this case.

[Robin Morisset]

<rdar://problem/37773612>

[Robin Morisset]

Created attachment 338767 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 338767 [details]:

imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight.any.html bug 185008 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 338767 [details]
Patch for landing

Clearing flags on attachment: 338767

Committed r231034: <https://trac.webkit.org/changeset/231034>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Robin Morisset]

(In reply to Filip Pizlo from comment #2)
> Comment on attachment 338320 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338320&action=review
> 
> R=me with that change.
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5284
> > +            RegisteredStructure structure = m_graph.registerStructure(globalObject->originalArrayStructureForIndexingType(ArrayWithContiguous));
> 
> I think it would be better if you added a JSGlobalObject helper method
> called originalRestParameterStructure. I think we have the
> restParameterStructure method to abstract the fact that it’s just the
> contiguous array structure.  It would be good if you also had such an
> abstraction for this case.

Thanks for the review, I did what you suggested and landed it.