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.
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 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.
(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.
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.
Patch no longer applies...
Let me rebase it
Created attachment 291604 [details] Rebased patch
Created attachment 291607 [details] Patch Now with the files added to XCode, thanks youenn!
Created attachment 291610 [details] Try to fix mac build
/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?
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.
Created attachment 291618 [details] Try to fix mac build Marking the header as private
(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.
Created attachment 291621 [details] Try to fix mac build
Created attachment 291624 [details] Try to fix mac build
Created attachment 291625 [details] Try to fix mac build
Created attachment 291627 [details] Try to fix mac build
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.
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
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.
Created attachment 291637 [details] Rebased again
Created attachment 291638 [details] Fix the build
ping reviewers.
I think Andreas Kling or Anders Carlsson should review this.
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???
Created attachment 294345 [details] Patch for landing
Committed r208534: <http://trac.webkit.org/changeset/208534>
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
We're calling WebInstallMemoryPressureHandler before [WebView initWithFrame:frameName:groupName:]
(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?
(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.
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?
(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 :-)
(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!
(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.
(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!