Bug 185231 – WebContent crash loading page on seas.upenn.edu @ JavaScriptCore: vmEntryToJavaScript

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

[Michael Saboff]

1. On an iOS device, load https://www.seas.upenn.edu/~cis194/fall16/hw/01-intro.html.
2. Zoom in & out and scroll up & down randomly.  Should crash fairly easily.

The issue is that we are not invalidating the dataMemoryTemp and cachedMemoryTemp registers when we transition masm.m_allowScratchRegister.  These transitions happen in the classes AllowMacroScratchRegisterUsage, AllowMacroScratchRegisterUsageIf and DisallowMacroScratchRegisterUsage.

[Michael Saboff]

<rdar://problem/39066024>

[Michael Saboff]

Created attachment 339423 [details]
Patch

[EWS Watchlist]

Attachment 339423 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/testair.cpp:1920:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 339423 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:10
> +        We weren't clearing the scratch register cache when switching back and forth between 
> +        allowing scratch register usage.  Now we clear when we transition from disallow to
> +        allow.

It's worth explaining why this is a problem.

> Source/JavaScriptCore/assembler/AllowMacroScratchRegisterUsageIf.h:45
> +#if CPU(ARM64)
> +            if (!m_oldValueOfAllowScratchRegister)
> +                m_masm.invalidateAllTempRegisters();
> +#endif

Shouldn't this invalidation happen on any transition?

[Michael Saboff]

(In reply to Saam Barati from comment #4)
> Comment on attachment 339423 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339423&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/ChangeLog:10
> > +        We weren't clearing the scratch register cache when switching back and forth between 
> > +        allowing scratch register usage.  Now we clear when we transition from disallow to
> > +        allow.
> 
> It's worth explaining why this is a problem.
> 
> > Source/JavaScriptCore/assembler/AllowMacroScratchRegisterUsageIf.h:45
> > +#if CPU(ARM64)
> > +            if (!m_oldValueOfAllowScratchRegister)
> > +                m_masm.invalidateAllTempRegisters();
> > +#endif
> 
> Shouldn't this invalidation happen on any transition?

Updated the ChangeLog to

We weren't clearing the scratch register cache when switching back and forth between allowing scratch register usage.  We disallow scratch register usage when we are in code that will freely allocate and use any register.  Such usage can change the contents of scratch registers.  For ARM64, where we cache the contents of scratch registers to reuse some or all of the contained values, we need to invalidate these caches.  We do this when re-enabling scratch register usage, that is when we transition from disallow to allow scratch register usage.

[EWS Watchlist]

Comment on attachment 339423 [details]
Patch

Attachment 339423 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7552870

New failing tests:
fast/mediastream/delayed-permission-allowed.html

[EWS Watchlist]

Created attachment 339434 [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

[Michael Saboff]

Committed r231317: <https://trac.webkit.org/changeset/231317>