Bug 185231 - WebContent crash loading page on seas.upenn.edu @ JavaScriptCore: vmEntryToJavaScript
Summary: WebContent crash loading page on seas.upenn.edu @ JavaScriptCore: vmEntryToJa...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-02 18:10 PDT by Michael Saboff
Modified: 2018-05-11 11:42 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.10 KB, patch)
2018-05-03 10:18 PDT, Michael Saboff
saam: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.90 MB, application/zip)
2018-05-03 11:21 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
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>