Bug 166875

Summary: Crash when WebCore's GC heap grows way too large.
Product: WebKit Reporter: Andreas Kling <kling>
Component: JavaScriptCoreAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, benjamin, cdumez, cmarcelo, commit-queue, dbates, fpizlo, ggaren, keith_miller, kling, mark.lam, msaboff, saam
Priority: P2 Keywords: Performance
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 166896    
Bug Blocks:    
Attachments:
Description Flags
Proposed patch
none
Proposed patch
mark.lam: review+
Patch for landing (Windows build fixed) none

Andreas Kling
Reported 2017-01-09 19:49:52 PST
<rdar://problem/27896585> Let's put a hard limit on the GC and crash instead of descending into swap hell.
Attachments
Proposed patch (2.17 KB, patch)
2017-01-09 20:38 PST, Andreas Kling 		no flags
DetailsFormatted DiffDiff
Proposed patch (5.02 KB, patch)
2017-01-11 10:24 PST, Andreas Kling 		mark.lam: review+
DetailsFormatted DiffDiff
Patch for landing (Windows build fixed) (4.90 KB, patch)
2017-01-11 12:09 PST, Andreas Kling 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Andreas Kling
Comment 1 2017-01-09 20:38:03 PST
Created attachment 298442 [details] Proposed patch
Mark Lam
Comment 2 2017-01-09 20:41:57 PST
Comment on attachment 298442 [details] Proposed patch r=me
WebKit Commit Bot
Comment 3 2017-01-10 04:17:37 PST
Comment on attachment 298442 [details] Proposed patch Clearing flags on attachment: 298442 Committed r210540: <http://trac.webkit.org/changeset/210540>
WebKit Commit Bot
Comment 4 2017-01-10 04:17:42 PST
All reviewed patches have been landed. Closing bug.
Filip Pizlo
Comment 5 2017-01-10 10:41:18 PST
Comment on attachment 298442 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=298442&action=review > Source/JavaScriptCore/ChangeLog:10 > + Hard cap the JavaScript heap at 4GB of live objects (determined post-GC.) > + If we go past this limit, crash with a recognizable signature. Why? This is a regression. If I have 20GB of RAM, why I can't I write a JavaScript program that uses all of it? Previously, I could. Now, I can't.
Filip Pizlo
Comment 6 2017-01-10 10:43:54 PST
If the point is to give a quota to WebProcesses, then the max heap size should be a parameter that WebCore passes to JSC. When JSC is used as a framework, embedded in who-knows-what, I don't think we should have any such limit.
Mark Lam
Comment 7 2017-01-10 10:47:15 PST
(In reply to comment #6) > If the point is to give a quota to WebProcesses, then the max heap size > should be a parameter that WebCore passes to JSC. > > When JSC is used as a framework, embedded in who-knows-what, I don't think > we should have any such limit. That's a very good point.
Andreas Kling
Comment 8 2017-01-10 11:09:35 PST
Yeah, fair point Phil. I will roll this out and give it a do-over.
WebKit Commit Bot
Comment 9 2017-01-10 11:12:16 PST
Re-opened since this is blocked by bug 166896
Andreas Kling
Comment 10 2017-01-11 10:24:30 PST
Created attachment 298597 [details] Proposed patch Only the common WebCore VM this time.
Mark Lam
Comment 11 2017-01-11 10:34:56 PST
Comment on attachment 298597 [details] Proposed patch r=me
Andreas Kling
Comment 12 2017-01-11 12:09:59 PST
Created attachment 298605 [details] Patch for landing (Windows build fixed)
Geoffrey Garen
Comment 13 2017-01-11 12:13:37 PST
Why not apply to the limit to the WebContent process instead?
Andreas Kling
Comment 14 2017-01-11 13:32:46 PST
(In reply to comment #13) > Why not apply to the limit to the WebContent process instead? The long-term plan is to integrate with OS facilities to do that (and more.) This patch is just a stopgap mitigation for a specific pathology.
WebKit Commit Bot
Comment 15 2017-01-11 16:56:36 PST
Comment on attachment 298605 [details] Patch for landing (Windows build fixed) Clearing flags on attachment: 298605 Committed r210609: <http://trac.webkit.org/changeset/210609>
WebKit Commit Bot
Comment 16 2017-01-11 16:56:42 PST
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.