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: JavaScriptCoreAssignee: 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: P2 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

Description Saam Barati 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.
Comment 1 Saam Barati 2016-06-28 10:41:29 PDT
<rdar://problem/26302360>
Comment 2 Filip Pizlo 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?
Comment 3 Saam Barati 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.
Comment 4 Saam Barati 2016-06-28 12:11:58 PDT
Created attachment 282265 [details]
patch
Comment 5 WebKit Commit Bot 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.
Comment 6 Filip Pizlo 2016-06-28 12:19:33 PDT
Comment on attachment 282265 [details]
patch

I can dig it.
Comment 7 Saam Barati 2016-06-28 12:23:23 PDT
Created attachment 282267 [details]
patch for landing if bots are happy
Comment 8 WebKit Commit Bot 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.
Comment 9 Saam Barati 2016-06-28 12:44:24 PDT
Created attachment 282269 [details]
lets see what the bots think
Comment 10 WebKit Commit Bot 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.
Comment 11 Saam Barati 2016-06-28 12:50:26 PDT
Created attachment 282272 [details]
lets see what the bots think
Comment 12 WebKit Commit Bot 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>
Comment 13 WebKit Commit Bot 2016-06-28 14:30:47 PDT
All reviewed patches have been landed.  Closing bug.