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

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

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.

[Saam Barati]

<rdar://problem/26302360>

[Filip Pizlo]

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

[Saam Barati]

(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]

Created attachment 282265 [details]
patch

[WebKit Commit Bot]

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 on attachment 282265 [details]
patch

I can dig it.

[Saam Barati]

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

[WebKit Commit Bot]

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]

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

[WebKit Commit Bot]

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]

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

[WebKit Commit Bot]

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]

All reviewed patches have been landed.  Closing bug.