Bug 189940 – Calls to baselineCodeBlockForOriginAndBaselineCodeBlock in operationMaterializeObjectInOSR should actually pass in the baseline CodeBlock

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

[Saam Barati]

We made a copy and paste error in the NewArrayBuffer case. That lead us to using the constant pool of the FTL code block instead of the baseline code block.

[Saam Barati]

I'm going to just make it so that we always pass in the baseline

[Saam Barati]

Created attachment 350726 [details]
patch

[Saam Barati]

<rdar://problem/43640987>

[Mark Lam]

Comment on attachment 350726 [details]
patch

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

r=me

> Source/JavaScriptCore/ChangeLog:14
> +        We were calling baselineCodeBlockForOriginAndBaselineCodeBlock with the FTL
> +        CodeBlock. There is nothing semantically wrong with doing that (except for
> +        poor naming), however, the poor naming here led us to make a real semantic
> +        mistake. We wanted the baseline CodeBlock's constant pool, but we were
> +        accessing the FTL CodeBlock's constant pool accidentally. We need to
> +        access the baseline CodeBlock's constant pool when we update the NewArrayBuffer
> +        constant value.

In baselineCodeBlockForOriginAndBaselineCodeBlock(), can we ASSERT(JITCode::isBaselineCode(baselineCodeBlock->jitType())?  That will help catch this error sooner in the future.

[Saam Barati]

Created attachment 350815 [details]
patch for landing

[Saam Barati]

(In reply to Mark Lam from comment #4)
> Comment on attachment 350726 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350726&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/ChangeLog:14
> > +        We were calling baselineCodeBlockForOriginAndBaselineCodeBlock with the FTL
> > +        CodeBlock. There is nothing semantically wrong with doing that (except for
> > +        poor naming), however, the poor naming here led us to make a real semantic
> > +        mistake. We wanted the baseline CodeBlock's constant pool, but we were
> > +        accessing the FTL CodeBlock's constant pool accidentally. We need to
> > +        access the baseline CodeBlock's constant pool when we update the NewArrayBuffer
> > +        constant value.
> 
> In baselineCodeBlockForOriginAndBaselineCodeBlock(), can we
> ASSERT(JITCode::isBaselineCode(baselineCodeBlock->jitType())?  That will
> help catch this error sooner in the future.

I spoke with Mark offline. We're going to add the assert.

[EWS Watchlist]

Comment on attachment 350815 [details]
patch for landing

Attachment 350815 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9350954

New failing tests:
fast/custom-elements/throw-on-dynamic-markup-insertion-counter-construct.html

[EWS Watchlist]

Created attachment 350832 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[WebKit Commit Bot]

Comment on attachment 350815 [details]
patch for landing

Clearing flags on attachment: 350815

Committed r236495: <https://trac.webkit.org/changeset/236495>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.