Bug 160497 – MemoryPressureHandler shouldn't know how to release WebCore memory

• Component: Platform
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

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.

[Andreas Kling]

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.

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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.

[youenn fablet]

Patch no longer applies...

[Carlos Garcia Campos]

Let me rebase it

[Carlos Garcia Campos]

Created attachment 291604 [details]
Rebased patch

[Carlos Garcia Campos]

Created attachment 291607 [details]
Patch

Now with the files added to XCode, thanks youenn!

[Carlos Garcia Campos]

Created attachment 291610 [details]
Try to fix mac build

[Carlos Garcia Campos]

/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?

[youenn fablet]

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.

[Carlos Garcia Campos]

Created attachment 291618 [details]
Try to fix mac build

Marking the header as private

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Created attachment 291621 [details]
Try to fix mac build

[Carlos Garcia Campos]

Created attachment 291624 [details]
Try to fix mac build

[Carlos Garcia Campos]

Created attachment 291625 [details]
Try to fix mac build

[Carlos Garcia Campos]

Created attachment 291627 [details]
Try to fix mac build

[Build Bot]

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.

[Build Bot]

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

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 291637 [details]
Rebased again

[Carlos Garcia Campos]

Created attachment 291638 [details]
Fix the build

[Carlos Garcia Campos]

ping reviewers.

[Darin Adler]

I think Andreas Kling or Anders Carlsson should review this.

[Michael Catanzaro]

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???

[Carlos Garcia Campos]

Created attachment 294345 [details]
Patch for landing

[Carlos Garcia Campos]

Committed r208534: <http://trac.webkit.org/changeset/208534>

[Alex Christensen]

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

[Alex Christensen]

We're calling WebInstallMemoryPressureHandler before [WebView initWithFrame:frameName:groupName:]

[Michael Catanzaro]

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

[Darin Adler]

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

[Alex Christensen]

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?

[Carlos Garcia Campos]

(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 :-)

[Michael Catanzaro]

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

[Darin Adler]

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

[Michael Catanzaro]

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