Bug 159198

Summary:

some Watchpoints' ::fireInternal method will call operations that might GC where the GC will cause the watchpoint itself to destruct

Product:

WebKit

Reporter:

Saam Barati <saam>

Component:

JavaScriptCore

Assignee:

Saam Barati <saam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, benjamin, cdumez, cgarcia, commit-queue, fpizlo, ggaren, gskachkov, keith_miller, malvika.editsoftdigital, mark.lam, msaboff, oliver, sukolsak, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
patch	fpizlo: review+
patch for landing if bots are happy	none
lets see what the bots think	none
lets see what the bots think	none

Saam Barati

Reported 2016-06-28 00:40:11 PDT

Watchpoints may lead to their own destruction under various scenarios where it’s not safe for them to die. The solution is to prevent GC from happeneing in these scenarios. Some watchpoints are smart enough to prevent GC from happening, but others have omitted this necessary code.

Attachments
patch (31.45 KB, patch) 2016-06-28 12:11 PDT, Saam Barati	fpizlo: review+	Details Formatted Diff Diff
patch for landing if bots are happy (31.44 KB, patch) 2016-06-28 12:23 PDT, Saam Barati	no flags	Details Formatted Diff Diff
lets see what the bots think (31.76 KB, patch) 2016-06-28 12:44 PDT, Saam Barati	no flags	Details Formatted Diff Diff
lets see what the bots think (31.76 KB, patch) 2016-06-28 12:50 PDT, Saam Barati	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Saam Barati

Comment 1 2016-06-28 10:41:29 PDT

<rdar://problem/26302360>

Filip Pizlo

Comment 2 2016-06-28 10:42:52 PDT

Does this mean that WatchpointSet::fireBlah should have a DeferGC in it, so that individual watchpoints don't have to do it?

Saam Barati

Comment 3 2016-06-28 10:48:18 PDT

(In reply to comment #2) > Does this mean that WatchpointSet::fireBlah should have a DeferGC in it, so > that individual watchpoints don't have to do it? I think that's how we should do it. This seems like the least error prone way to future proof new ::fireInternal implementations.

Saam Barati

Comment 4 2016-06-28 12:11:58 PDT

Created attachment 282265 [details] patch

WebKit Commit Bot

Comment 5 2016-06-28 12:13:05 PDT

Attachment 282265 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/VariableWriteFireDetail.h:46: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/ChangeLog:9: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] ERROR: Source/JavaScriptCore/bytecode/Watchpoint.h:320: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 6 2016-06-28 12:19:33 PDT

Comment on attachment 282265 [details] patch I can dig it.

Saam Barati

Comment 7 2016-06-28 12:23:23 PDT

Created attachment 282267 [details] patch for landing if bots are happy

WebKit Commit Bot

Comment 8 2016-06-28 12:26:30 PDT

Attachment 282267 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/VariableWriteFireDetail.h:46: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/bytecode/Watchpoint.h:320: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.

Saam Barati

Comment 9 2016-06-28 12:44:24 PDT

Created attachment 282269 [details] lets see what the bots think

WebKit Commit Bot

Comment 10 2016-06-28 12:46:17 PDT

Attachment 282269 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/Watchpoint.cpp:29: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.

Saam Barati

Comment 11 2016-06-28 12:50:26 PDT

Created attachment 282272 [details] lets see what the bots think

WebKit Commit Bot

Comment 12 2016-06-28 14:30:41 PDT

Comment on attachment 282272 [details] lets see what the bots think Clearing flags on attachment: 282272 Committed r202588: <http://trac.webkit.org/changeset/202588>

WebKit Commit Bot

Comment 13 2016-06-28 14:30:47 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.