Bug 165483

Summary: REGRESSION(r209399): Causes crashes when dumping JIT disassembly
Product: WebKit Reporter: Michael Saboff <msaboff>
Component: JavaScriptCoreAssignee: Michael Saboff <msaboff>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, darin, dbates, ryanhaddad
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Michael Saboff
Reported 2016-12-06 12:21:14 PST
Looks like the RELEASE_ASSERT() at the end of sixCharacterHashStringToInteger() has an inverted check. We want to assert that the string is terminated with a null character.
Attachments
Patch (1.22 KB, patch)
2016-12-06 12:25 PST, Michael Saboff 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Michael Saboff
Comment 1 2016-12-06 12:25:18 PST
Created attachment 296306 [details] Patch
Geoffrey Garen
Comment 2 2016-12-06 12:27:06 PST
Comment on attachment 296306 [details] Patch r=me
Darin Adler
Comment 3 2016-12-06 12:58:45 PST
Comment on attachment 296306 [details] Patch Oops! Thanks very much for fixing this.
Darin Adler
Comment 4 2016-12-06 12:59:10 PST
Wonder why EWS missed it.
Michael Saboff
Comment 5 2016-12-06 13:01:23 PST
(In reply to comment #4) > Wonder why EWS missed it. I don't think the debug tests exercise this path. That would require something to dump the hashed name of a compiled JS function.
WebKit Commit Bot
Comment 6 2016-12-06 13:04:10 PST
Comment on attachment 296306 [details] Patch Clearing flags on attachment: 296306 Committed r209413: <http://trac.webkit.org/changeset/209413>
WebKit Commit Bot
Comment 7 2016-12-06 13:04:15 PST
All reviewed patches have been landed. Closing bug.
Ryan Haddad
Comment 8 2016-12-06 14:08:41 PST
*** Bug 165490 has been marked as a duplicate of this bug. ***
Michael Saboff
Comment 9 2016-12-06 14:11:25 PST
(In reply to comment #5) > (In reply to comment #4) > > Wonder why EWS missed it. > > I don't think the debug tests exercise this path. That would require > something to dump the hashed name of a compiled JS function. I was wrong, the debug bots did catch this (https://bugs.webkit.org/show_bug.cgi?id=165490). It just took a little time.
Darin Adler
Comment 10 2016-12-06 18:51:00 PST
Are "the debug bots" part of EWS, or part of some other automated WebKit testing?
Darin Adler
Comment 11 2016-12-06 18:51:40 PST
Also, Michael, do you know why these need to be RELEASE_ASSERT and not just ASSERT?
Michael Saboff
Comment 12 2016-12-07 10:05:25 PST
(In reply to comment #10) > Are "the debug bots" part of EWS, or part of some other automated WebKit > testing? There is a mac-debug EWS bot. I don't know if this bot would / did catch the issue. It would depend on what tests that bot runs. > Also, Michael, do you know why these need to be RELEASE_ASSERT and not just ASSERT? I don't know why this isn't simply an ASSERT.
Ryan Haddad
Comment 13 2016-12-07 13:15:57 PST
(In reply to comment #12) > (In reply to comment #10) > > Are "the debug bots" part of EWS, or part of some other automated WebKit > > testing? > > There is a mac-debug EWS bot. I don't know if this bot would / did catch > the issue. It would depend on what tests that bot runs. The failure was caught after the patch was landed by the bots that run JSC tests. EWS does not currently run JSC tests.
Note You need to log in before you can comment on or make changes to this bug.