Bug 160497 - MemoryPressureHandler shouldn't know how to release WebCore memory
Summary: MemoryPressureHandler shouldn't know how to release WebCore memory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-03 04:53 PDT by Carlos Garcia Campos
Modified: 2016-12-19 21:01 PST (History)
7 users (show)

See Also:


Attachments
WIP patch (35.52 KB, patch)
2016-08-03 05:08 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased patch (35.50 KB, patch)
2016-09-19 05:57 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased patch (35.49 KB, patch)
2016-10-14 02:48 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (41.34 KB, patch)
2016-10-14 03:32 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix mac build (41.51 KB, patch)
2016-10-14 04:06 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix mac build (41.55 KB, patch)
2016-10-14 04:55 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix mac build (41.54 KB, patch)
2016-10-14 05:28 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix mac build (41.57 KB, patch)
2016-10-14 05:48 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix mac build (41.57 KB, patch)
2016-10-14 06:11 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix mac build (41.88 KB, patch)
2016-10-14 06:27 PDT, Carlos Garcia Campos
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-yosemite (872.87 KB, application/zip)
2016-10-14 07:20 PDT, Build Bot
no flags Details
Try to fix wk1 crashes (42.00 KB, patch)
2016-10-14 08:42 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased again (42.30 KB, patch)
2016-10-14 08:54 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Fix the build (42.33 KB, patch)
2016-10-14 09:01 PDT, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff
Patch for landing (42.23 KB, patch)
2016-11-10 01:18 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2016-08-03 04:53:27 PDT
All processes should set their own low memory handler, instead of leaving the web process using the default one that needs to access APIs that are not in platform layer.
Comment 1 Carlos Garcia Campos 2016-08-03 05:08:08 PDT
Created attachment 285219 [details]
WIP patch

It's actually finished, it's WIP because it needs the XCode changes, but it would be great if could be reviewed before I find someone who add the new files for me. This should fix all layergin violations in MemoryPressureHandler.
Comment 2 Andreas Kling 2016-08-05 07:40:46 PDT
Comment on attachment 285219 [details]
WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=285219&action=review

This is quite nice :)

> Source/WebKit/mac/WebView/WebView.mm:1127
> +        notify_register_dispatch("com.apple.WebKit.fullGC", &dummy, dispatch_get_main_queue(), ^(int) {
> +            GCController::singleton().garbageCollectNow();
> +        });
> +        notify_register_dispatch("com.apple.WebKit.deleteAllCode", &dummy, dispatch_get_main_queue(), ^(int) {
> +            GCController::singleton().deleteAllCode();
> +            GCController::singleton().garbageCollectNow();
> +        });

This code is duplicated in both WebKit1 and WebKit2. If a process uses both, it will hook 2 separate callbacks to each notification name, effectively making the callbacks execute twice.
Comment 3 Carlos Garcia Campos 2016-09-19 05:37:22 PDT
(In reply to comment #2)
> Comment on attachment 285219 [details]
> WIP patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285219&action=review
> 
> This is quite nice :)

Thanks, I still need help with the XCode changes.

> > Source/WebKit/mac/WebView/WebView.mm:1127
> > +        notify_register_dispatch("com.apple.WebKit.fullGC", &dummy, dispatch_get_main_queue(), ^(int) {
> > +            GCController::singleton().garbageCollectNow();
> > +        });
> > +        notify_register_dispatch("com.apple.WebKit.deleteAllCode", &dummy, dispatch_get_main_queue(), ^(int) {
> > +            GCController::singleton().deleteAllCode();
> > +            GCController::singleton().garbageCollectNow();
> > +        });
> 
> This code is duplicated in both WebKit1 and WebKit2. If a process uses both,
> it will hook 2 separate callbacks to each notification name, effectively
> making the callbacks execute twice.

Is it really possible to use WebKit1 and WebKit2 from the same process? The WebKit2 code is in the web process initialization, so even if an application uses both, the UI process would install the WebKit1 callbacks and the web processes the WebKit2 ones. So, I'm not sure this is a problem. It would only be a problem if webkit1 can be used from a webkit2 injected bundle or something like that.
Comment 4 Carlos Garcia Campos 2016-09-19 05:57:57 PDT
Created attachment 289224 [details]
Rebased patch

Patch rebased to apply on current trunk. Moved the notify callback registration to a common place in webcore and ensured they are only registered once.
Comment 5 youenn fablet 2016-10-14 02:35:13 PDT
Patch no longer applies...
Comment 6 Carlos Garcia Campos 2016-10-14 02:41:02 PDT
Let me rebase it
Comment 7 Carlos Garcia Campos 2016-10-14 02:48:13 PDT
Created attachment 291604 [details]
Rebased patch
Comment 8 Carlos Garcia Campos 2016-10-14 03:32:36 PDT
Created attachment 291607 [details]
Patch

Now with the files added to XCode, thanks youenn!
Comment 9 Carlos Garcia Campos 2016-10-14 04:06:01 PDT
Created attachment 291610 [details]
Try to fix mac build
Comment 10 Carlos Garcia Campos 2016-10-14 04:41:51 PDT
/Volumes/Data/EWS/WebKit/Source/WebKit/mac/WebView/WebView.mm:160:9: fatal error: 'WebCore/MemoryRelease.h' file not found

Do I need to do anything special for wk1 to find that WebCore header?
Comment 11 youenn fablet 2016-10-14 04:47:26 PDT
Maybe marking it as Private in Xcode file.
Edit the pbxproj file and add something like
"settings = {ATTRIBUTES = (Private, );"
to one of the added line for MemoryRelease.h that also has "fileRef" in it.
Comment 12 Carlos Garcia Campos 2016-10-14 04:55:43 PDT
Created attachment 291618 [details]
Try to fix mac build

Marking the header as private
Comment 13 Carlos Garcia Campos 2016-10-14 04:56:11 PDT
(In reply to comment #11)
> Maybe marking it as Private in Xcode file.
> Edit the pbxproj file and add something like
> "settings = {ATTRIBUTES = (Private, );"
> to one of the added line for MemoryRelease.h that also has "fileRef" in it.

Trying that, thanks again.
Comment 14 Carlos Garcia Campos 2016-10-14 05:28:39 PDT
Created attachment 291621 [details]
Try to fix mac build
Comment 15 Carlos Garcia Campos 2016-10-14 05:48:52 PDT
Created attachment 291624 [details]
Try to fix mac build
Comment 16 Carlos Garcia Campos 2016-10-14 06:11:36 PDT
Created attachment 291625 [details]
Try to fix mac build
Comment 17 Carlos Garcia Campos 2016-10-14 06:27:08 PDT
Created attachment 291627 [details]
Try to fix mac build
Comment 18 Build Bot 2016-10-14 07:20:13 PDT
Comment on attachment 291627 [details]
Try to fix mac build

Attachment 291627 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2284493

Number of test failures exceeded the failure limit.
Comment 19 Build Bot 2016-10-14 07:20:18 PDT
Created attachment 291632 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 20 Carlos Garcia Campos 2016-10-14 08:42:23 PDT
Created attachment 291636 [details]
Try to fix wk1 crashes

Ensure we install the memory pressure only once in WebKit1 mac to avoid hitting an assert.
Comment 21 Carlos Garcia Campos 2016-10-14 08:54:36 PDT
Created attachment 291637 [details]
Rebased again
Comment 22 Carlos Garcia Campos 2016-10-14 09:01:50 PDT
Created attachment 291638 [details]
Fix the build
Comment 23 Carlos Garcia Campos 2016-10-20 05:53:55 PDT
ping reviewers.
Comment 24 Darin Adler 2016-10-20 09:38:00 PDT
I think Andreas Kling or Anders Carlsson should review this.
Comment 25 Michael Catanzaro 2016-11-01 16:53:51 PDT
Comment on attachment 291638 [details]
Fix the build

View in context: https://bugs.webkit.org/attachment.cgi?id=291638&action=review

Andreas liked the initial version of the patch (see comment #2), and I think not much has changed; I think we can count that as owner approval if there are not any objections. It looks like a good fix for these layering violations. A couple questions:

> Source/WebCore/platform/MemoryPressureHandler.cpp:62
> +    if (!m_lowMemoryHandler)
> +        return;

Wouldn't an early return here be a bug? Shouldn't there be an assertion here?

> Source/WebCore/platform/MemoryPressureHandler.h:173
> +    void (^m_releaseMemoryBlock)() { nullptr };

I know this was here before, but... what is this???
Comment 26 Carlos Garcia Campos 2016-11-10 01:18:12 PST
Created attachment 294345 [details]
Patch for landing
Comment 27 Carlos Garcia Campos 2016-11-10 06:27:42 PST
Committed r208534: <http://trac.webkit.org/changeset/208534>
Comment 28 Alex Christensen 2016-11-10 18:35:12 PST
Comment on attachment 294345 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=294345&action=review

> Source/WebCore/platform/MemoryPressureHandler.h:72
>          ASSERT(!m_installed);

I'm hitting this assertion now in debug Safari
Comment 29 Alex Christensen 2016-11-10 18:38:39 PST
We're calling WebInstallMemoryPressureHandler before [WebView initWithFrame:frameName:groupName:]
Comment 30 Michael Catanzaro 2016-11-10 18:47:08 PST
(In reply to comment #29)
> We're calling WebInstallMemoryPressureHandler before [WebView
> initWithFrame:frameName:groupName:]

WebInstallMemoryPressureHandler only exists in WebKit1. Surely Safari is not using it?
Comment 31 Darin Adler 2016-11-10 20:39:33 PST
(In reply to comment #30)
> (In reply to comment #29)
> > We're calling WebInstallMemoryPressureHandler before [WebView
> > initWithFrame:frameName:groupName:]
> 
> WebInstallMemoryPressureHandler only exists in WebKit1. Surely Safari is not
> using it?

Safari uses both legacy WebKit (WebKit1) and modern WebKit (WebKit2). Some day it might get to the point where it has no dependency on legacy WebKit, but at the moment it still uses both.
Comment 32 Alex Christensen 2016-11-10 22:19:44 PST
I fixed it in http://trac.webkit.org/changeset/208580
Carlos and Michael, could you look at that change and make sure I'm not abusing your work?
Comment 33 Carlos Garcia Campos 2016-11-10 22:57:21 PST
(In reply to comment #32)
> I fixed it in http://trac.webkit.org/changeset/208580
> Carlos and Michael, could you look at that change and make sure I'm not
> abusing your work?

Great, thanks for fixing it without rolling out the patch :-)
Comment 34 Michael Catanzaro 2016-11-11 04:38:17 PST
(In reply to comment #31)
> Safari uses both legacy WebKit (WebKit1) and modern WebKit (WebKit2). Some
> day it might get to the point where it has no dependency on legacy WebKit,
> but at the moment it still uses both.

Anywhere I can learn about why Safari still needs legacy WebKit and what it uses it for? That's really surprising to me and I'm curious!
Comment 35 Darin Adler 2016-11-11 15:18:40 PST
(In reply to comment #34)
> (In reply to comment #31)
> > Safari uses both legacy WebKit (WebKit1) and modern WebKit (WebKit2). Some
> > day it might get to the point where it has no dependency on legacy WebKit,
> > but at the moment it still uses both.
> 
> Anywhere I can learn about why Safari still needs legacy WebKit and what it
> uses it for? That's really surprising to me and I'm curious!

I don’t think there is anywhere to learn that, but I am willing to disclose a little bit: At some point in the recent past, there was some use of legacy WebKit in some of the Safari Browser Extension support.

Also, keep in mind that WebKit is a system framework on macOS; Safari can invoke some other macOS framework and nothing prevents that framework from using some bit of WebKit API for some task.
Comment 36 Michael Catanzaro 2016-12-19 21:01:53 PST
(Note: I've removed this patch from the list of proposed backports for 2.14 because it is a huge refactor. r208975 can still easily be backported, the code just moved from one file to another.)

(In reply to comment #35)
> I don’t think there is anywhere to learn that, but I am willing to disclose
> a little bit: At some point in the recent past, there was some use of legacy
> WebKit in some of the Safari Browser Extension support.
> 
> Also, keep in mind that WebKit is a system framework on macOS; Safari can
> invoke some other macOS framework and nothing prevents that framework from
> using some bit of WebKit API for some task.

Interesting!