Bug 159198 - some Watchpoints' ::fireInternal method will call operations that might GC where the GC will cause the watchpoint itself to destruct
Summary: some Watchpoints' ::fireInternal method will call operations that might GC wh...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-06-28 00:40 PDT by Saam Barati
Modified: 2019-05-02 16:22 PDT (History)
16 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.