Bug 162721 - REGRESSION(r206555): It made Dromaeo/jslib-style-jquery.html crash
Summary: REGRESSION(r206555): It made Dromaeo/jslib-style-jquery.html crash
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks: 162316
  Show dependency treegraph
 
Reported: 2016-09-29 03:01 PDT by Csaba Osztrogonác
Modified: 2016-09-29 17:51 PDT (History)
9 users (show)

See Also:


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+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Csaba Osztrogonác 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
Comment 2 Csaba Osztrogonác 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
Comment 3 Csaba Osztrogonác 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.
Comment 4 Filip Pizlo 2016-09-29 09:26:44 PDT
I will look!
Comment 5 Ryan Haddad 2016-09-29 09:30:32 PDT
Created attachment 290206 [details]
Crashlog

Crashlog from El Capitan perf bot.
Comment 6 Filip Pizlo 2016-09-29 11:10:38 PDT
I can repro in minibrowser.
Comment 7 Filip Pizlo 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.
Comment 8 Filip Pizlo 2016-09-29 12:00:13 PDT
It looks as though the crash happens with the DFG JIT disabled.
Comment 9 Filip Pizlo 2016-09-29 12:07:55 PDT
This seems to require the baseline JIT.
Comment 10 Filip Pizlo 2016-09-29 12:14:37 PDT
I have a theory about what it is.  I'm testing it now.
Comment 11 Filip Pizlo 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.
Comment 12 Filip Pizlo 2016-09-29 14:00:20 PDT
Looks like this has something to do with put_by_val.
Comment 13 Filip Pizlo 2016-09-29 17:22:18 PDT
I have a fix!!!
Comment 14 Filip Pizlo 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.
Comment 15 Filip Pizlo 2016-09-29 17:35:49 PDT
Created attachment 290272 [details]
the patch
Comment 16 Keith Miller 2016-09-29 17:37:17 PDT
Comment on attachment 290272 [details]
the patch

r=me.
Comment 17 Filip Pizlo 2016-09-29 17:51:59 PDT
Landed in https://trac.webkit.org/changeset/206628