Bug 185231

Summary: WebContent crash loading page on seas.upenn.edu @ JavaScriptCore: vmEntryToJavaScript
Product: WebKit Reporter: Michael Saboff <msaboff>
Component: JavaScriptCoreAssignee: Michael Saboff <msaboff>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, rniwa, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=185554
Attachments:
Description Flags
Patch
sbarati: review+, ews-watchlist: commit-queue-
Archive of layout-test-results from ews104 for mac-sierra-wk2 none

Description Michael Saboff 2018-05-02 18:10:41 PDT
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.
Comment 1 Michael Saboff 2018-05-02 18:11:11 PDT
<rdar://problem/39066024>
Comment 2 Michael Saboff 2018-05-03 10:18:36 PDT
Created attachment 339423 [details]
Patch
Comment 3 EWS Watchlist 2018-05-03 10:22:06 PDT
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.
Comment 4 Saam Barati 2018-05-03 10:22:35 PDT
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?
Comment 5 Michael Saboff 2018-05-03 10:33:56 PDT
(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.
Comment 6 EWS Watchlist 2018-05-03 11:21:56 PDT
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
Comment 7 EWS Watchlist 2018-05-03 11:21:57 PDT
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
Comment 8 Michael Saboff 2018-05-03 11:39:15 PDT
Committed r231317: <https://trac.webkit.org/changeset/231317>