Bug 109103 – PluginProcess should quit immediately if idle in response to low-memory notifications

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Gavin Barraclough]

Summary really says it all.

[Gavin Barraclough]

Created attachment 186954 [details]
fix

[Darin Adler]

Comment on attachment 186954 [details]
fix

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

> Source/WebCore/platform/MemoryPressureHandler.cpp:55
>  void MemoryPressureHandler::respondToMemoryPressure() { }
> +
> +void MemoryPressureHandler::releaseMemory(bool) { }
>  #endif

Space between functions is strange if they are going to be all on one line like this. Lack of space between #if/#endif and the functions is stranger.

> Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:137
> +    m_lowMemoryHandler(false);

Can m_lowMemoryHandler be null here? If not why not?

Separate thought: Boolean constants stink at call sites like this.

> Source/WebKit2/PluginProcess/PluginProcess.h:112
> +    static void lowMemoryHandler(bool);

Need an argument name here. It’s not at all clear what the bool is for.

[Gavin Barraclough]

(In reply to comment #2)
> (From update of attachment 186954 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186954&action=review
> 
> > Source/WebCore/platform/MemoryPressureHandler.cpp:55
> >  void MemoryPressureHandler::respondToMemoryPressure() { }
> > +
> > +void MemoryPressureHandler::releaseMemory(bool) { }
> >  #endif
> 
> Space between functions is strange if they are going to be all on one line like this. Lack of space between #if/#endif and the functions is stranger.

Will fix.

> > Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:137
> > +    m_lowMemoryHandler(false);
> 
> Can m_lowMemoryHandler be null here? If not why not?

No cannot be null, m_lowMemoryHandler is always set by initialize, before install is called - and install is private.
I'll add an ASSERT.

> Separate thought: Boolean constants stink at call sites like this.

True - worse still, this boolean is only ever false. :-(
I didn't want to change as a part of this patch, I'll follow up separately on this.

> > Source/WebKit2/PluginProcess/PluginProcess.h:112
> > +    static void lowMemoryHandler(bool);
> 
> Need an argument name here. It’s not at all clear what the bool is for.

Will do.

[Gavin Barraclough]

Fixed in r142212

[Takashi Sakamoto]

I think, the ASSERT(!m_installed) causes LayoutTests/inspector/* crash.
The following backtraces show where MemoryPressureHandler()::intialize is invoked from.

The first "initialize":
(lldb) thread backtrace
* thread #1: tid = 0x1c03, 0x000000010158ddd3 WebKit`WebCore::MemoryPressureHandler::initialize(void (*)(bool)) + 51 at MemoryPressureHandler.h:44, stop reason = breakpoint 1.1
    frame #0: 0x000000010158ddd3 WebKit`WebCore::MemoryPressureHandler::initialize(void (*)(bool)) + 51 at MemoryPressureHandler.h:44
    frame #1: 0x000000010156bb81 WebKit`-[WebView(WebPrivate) _commonInitializationWithFrameName:groupName:] + 3985 at WebView.mm:822
    frame #2: 0x000000010156c0eb WebKit`-[WebView(WebPrivate) _initWithFrame:frameName:groupName:usesDocumentViews:] + 459 at WebView.mm:867
    frame #3: 0x0000000101578fb4 WebKit`-[WebView initWithFrame:frameName:groupName:] + 308 at WebView.mm:3407
    frame #4: 0x0000000100016151 DumpRenderTree`createWebViewAndOffscreenWindow() + 321 at DumpRenderTree.mm:526
    frame #5: 0x000000010001684f DumpRenderTree`dumpRenderTree(int, char const**) + 127 at DumpRenderTree.mm:869
    frame #6: 0x0000000100018bd9 DumpRenderTree`main + 105 at DumpRenderTree.mm:924
    frame #7: 0x00007fff85d537e1 libdyld.dylib`start + 1

The second "initialize", invoked from inspector.
(lldb) thread backtrace
* thread #1: tid = 0x1c03, 0x000000010158ddd3 WebKit`WebCore::MemoryPressureHandler::initialize(void (*)(bool)) + 51 at MemoryPressureHandler.h:44, stop reason = breakpoint 1.1
    frame #0: 0x000000010158ddd3 WebKit`WebCore::MemoryPressureHandler::initialize(void (*)(bool)) + 51 at MemoryPressureHandler.h:44
    frame #1: 0x000000010156bb81 WebKit`-[WebView(WebPrivate) _commonInitializationWithFrameName:groupName:] + 3985 at WebView.mm:822
    frame #2: 0x000000010156c0eb WebKit`-[WebView(WebPrivate) _initWithFrame:frameName:groupName:usesDocumentViews:] + 459 at WebView.mm:867
    frame #3: 0x0000000101578fb4 WebKit`-[WebView initWithFrame:frameName:groupName:] + 308 at WebView.mm:3407
    frame #4: 0x0000000101578e75 WebKit`-[WebView initWithFrame:] + 133 at WebView.mm:3398
    frame #5: 0x00007fff88a10148 AppKit`-[NSView init] + 62
    frame #6: 0x00000001014f54fc WebKit`-[WebInspectorWindowController init] + 556 at WebInspectorClient.mm:371
    frame #7: 0x00000001014f5777 WebKit`-[WebInspectorWindowController initWithInspectedWebView:] + 39 at WebInspectorClient.mm:390
    frame #8: 0x00000001014f3768 WebKit`WebInspectorClient::openInspectorFrontend(WebCore::InspectorController*) + 88 at WebInspectorClient.mm:138
    frame #9: 0x00000001025d3364 WebCore`WebCore::InspectorController::show() + 116 at InspectorController.cpp:275
    frame #10: 0x00000001014f2747 WebKit`-[WebInspector showWindow] + 71 at WebInspector.mm:66
    frame #11: 0x00000001014f2777 WebKit`-[WebInspector show:] + 39 at WebInspector.mm:71
    frame #12: 0x0000000100059505 DumpRenderTree`TestRunner::showWebInspector() + 85 at TestRunnerMac.mm:850
    frame #13: 0x0000000100017eb5 DumpRenderTree`runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 3781 at DumpRenderTree.mm:1347
    frame #14: 0x0000000100016a1d DumpRenderTree`dumpRenderTree(int, char const**) + 589 at DumpRenderTree.mm:890
    frame #15: 0x0000000100018bd9 DumpRenderTree`main + 105 at DumpRenderTree.mm:924
    frame #16: 0x00007fff85d537e1 libdyld.dylib`start + 1

[Simon Fraser (smfr)]

I see that assertion too. I'm going to revert this.

[WebKit Review Bot]

Re-opened since this is blocked by bug 109255

[Simon Fraser (smfr)]

This also asserts in Safari.

[Gavin Barraclough]

Created attachment 187651 [details]
Fix take 2

[Gavin Barraclough]

Relanded in r142521