Bug 162721

Summary: REGRESSION(r206555): It made Dromaeo/jslib-style-jquery.html crash
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: New BugsAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fpizlo, ggaren, keith_miller, mark.lam, msaboff, ossy, ryanhaddad, saam
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 162316    
Attachments:
Description Flags
Crashlog
none
the patch keith_miller: review+

Attachments
Crashlog (72.72 KB, text/plain)
2016-09-29 09:30 PDT, Ryan Haddad 		no flags
Details
the patch (1.89 KB, patch)
2016-09-29 17:35 PDT, Filip Pizlo 		keith_miller: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Csaba Osztrogonác
Comment 1 2016-09-29 03:02:55 PDT
forced perf test on r206552 to bisect this bug: https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2%20%28Perf%29/builds/10051
Csaba Osztrogonác
Comment 2 2016-09-29 03:04:53 PDT
I think one of the following change caused this regression: The write barrier should be down with TSO ​https://bugs.webkit.org/show_bug.cgi?id=162316 https://trac.webkit.org/changeset/206555 Fix race condition in StringView's UnderlyingString lifecycle management. ​https://bugs.webkit.org/show_bug.cgi?id=162702 https://trac.webkit.org/changeset/206552
Csaba Osztrogonác
Comment 3 2016-09-29 05:44:15 PDT
(In reply to comment #1) > forced perf test on r206552 to bisect this bug: > https://build.webkit.org/builders/EFL%20Linux%2064- > bit%20Release%20WK2%20%28Perf%29/builds/10051 r206552 is good, r206553 and r206554 are unrelated change, so r206555 is the culprit.
Filip Pizlo
Comment 4 2016-09-29 09:26:44 PDT
I will look!
Ryan Haddad
Comment 5 2016-09-29 09:30:32 PDT
Created attachment 290206 [details] Crashlog Crashlog from El Capitan perf bot.
Filip Pizlo
Comment 6 2016-09-29 11:10:38 PDT
I can repro in minibrowser.
Filip Pizlo
Comment 7 2016-09-29 11:48:42 PDT
Looks like this is a case of a missing barrier, since the crash does not happen with gengc disabled.
Filip Pizlo
Comment 8 2016-09-29 12:00:13 PDT
It looks as though the crash happens with the DFG JIT disabled.
Filip Pizlo
Comment 9 2016-09-29 12:07:55 PDT
This seems to require the baseline JIT.
Filip Pizlo
Comment 10 2016-09-29 12:14:37 PDT
I have a theory about what it is. I'm testing it now.
Filip Pizlo
Comment 11 2016-09-29 12:34:09 PDT
Nope, still crashes. I thought it was because put_by_id's slow path was sometimes linking to after the barrier, but that's not the problem.
Filip Pizlo
Comment 12 2016-09-29 14:00:20 PDT
Looks like this has something to do with put_by_val.
Filip Pizlo
Comment 13 2016-09-29 17:22:18 PDT
I have a fix!!!
Filip Pizlo
Comment 14 2016-09-29 17:24:29 PDT
The problem is that the barrier in the put_by_id-in-put_by_val thing (JIT::privateCompileGetByValWithCachedId) did its write barrier all wrong. The fix is easy but I need to test a lot of things. ETA for patch 30 mins.
Filip Pizlo
Comment 15 2016-09-29 17:35:49 PDT
Created attachment 290272 [details] the patch
Keith Miller
Comment 16 2016-09-29 17:37:17 PDT
Comment on attachment 290272 [details] the patch r=me.
Filip Pizlo
Comment 17 2016-09-29 17:51:59 PDT
Landed in https://trac.webkit.org/changeset/206628
Note You need to log in before you can comment on or make changes to this bug.