Bug 184773

Summary: In FTLLowerDFGToB3.cpp::compileCreateRest, always use a contiguous array as the indexing type when under isWatchingHavingABadTimeWatchpoint
Product: WebKit Reporter: Robin Morisset <rmorisset>
Component: JavaScriptCoreAssignee: Robin Morisset <rmorisset>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, saam
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing none

Robin Morisset
Reported 2018-04-19 06:34:33 PDT
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>
Attachments
Patch (4.18 KB, patch)
2018-04-19 06:46 PDT, Robin Morisset 		no flags
DetailsFormatted DiffDiff
Patch for landing (5.06 KB, patch)
2018-04-25 12:00 PDT, Robin Morisset 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Robin Morisset
Comment 1 2018-04-19 06:46:11 PDT
Created attachment 338320 [details] Patch
Filip Pizlo
Comment 2 2018-04-23 06:45:05 PDT
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
Comment 3 2018-04-25 11:45:51 PDT
<rdar://problem/37773612>
Robin Morisset
Comment 4 2018-04-25 12:00:11 PDT
Created attachment 338767 [details] Patch for landing
WebKit Commit Bot
Comment 5 2018-04-25 16:32:26 PDT
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: youennf@gmail.com) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 6 2018-04-25 16:33:04 PDT
Comment on attachment 338767 [details] Patch for landing Clearing flags on attachment: 338767 Committed r231034: <https://trac.webkit.org/changeset/231034>
WebKit Commit Bot
Comment 7 2018-04-25 16:33:05 PDT
All reviewed patches have been landed. Closing bug.
Robin Morisset
Comment 8 2018-04-26 05:42:15 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.