Bug 166875 - Crash when WebCore's GC heap grows way too large.
Summary: Crash when WebCore's GC heap grows way too large.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords: Performance
Depends on: 166896
Blocks:
  Show dependency treegraph
 
Reported: 2017-01-09 19:49 PST by Andreas Kling
Modified: 2017-01-11 16:56 PST (History)
13 users (show)

See Also:


Attachments
Proposed patch (2.17 KB, patch)
2017-01-09 20:38 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Proposed patch (5.02 KB, patch)
2017-01-11 10:24 PST, Andreas Kling
mark.lam: review+
Details | Formatted Diff | Diff
Patch for landing (Windows build fixed) (4.90 KB, patch)
2017-01-11 12:09 PST, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 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.
Comment 1 Andreas Kling 2017-01-09 20:38:03 PST
Created attachment 298442 [details]
Proposed patch
Comment 2 Mark Lam 2017-01-09 20:41:57 PST
Comment on attachment 298442 [details]
Proposed patch

r=me
Comment 3 WebKit Commit Bot 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>
Comment 4 WebKit Commit Bot 2017-01-10 04:17:42 PST
All reviewed patches have been landed.  Closing bug.
Comment 5 Filip Pizlo 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.
Comment 6 Filip Pizlo 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.
Comment 7 Mark Lam 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.
Comment 8 Andreas Kling 2017-01-10 11:09:35 PST
Yeah, fair point Phil. I will roll this out and give it a do-over.
Comment 9 WebKit Commit Bot 2017-01-10 11:12:16 PST
Re-opened since this is blocked by bug 166896
Comment 10 Andreas Kling 2017-01-11 10:24:30 PST
Created attachment 298597 [details]
Proposed patch

Only the common WebCore VM this time.
Comment 11 Mark Lam 2017-01-11 10:34:56 PST
Comment on attachment 298597 [details]
Proposed patch

r=me
Comment 12 Andreas Kling 2017-01-11 12:09:59 PST
Created attachment 298605 [details]
Patch for landing (Windows build fixed)
Comment 13 Geoffrey Garen 2017-01-11 12:13:37 PST
Why not apply to the limit to the WebContent process instead?
Comment 14 Andreas Kling 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.
Comment 15 WebKit Commit Bot 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>
Comment 16 WebKit Commit Bot 2017-01-11 16:56:42 PST
All reviewed patches have been landed.  Closing bug.