Bug 181771 – REGRESSION (r223149): WebProcessProxy::didClose() no longer refs WebPageProxy objects

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

WebProcessProxy::didClose() no longer refs WebPageProxy objects, leading to crashes:
Thread[0] EXC_BAD_ACCESS (SIGSEGV) (KERN_INVALID_ADDRESS at 0x0000000200000353)
[  0] 0x00000001945ccf58 WebKit`WebKit::WebPageProxy::processDidTerminate(WebKit::ProcessTerminationReason) [inlined] WebKit::WebProcessProxy::isUnderMemoryPressure() const at WebProcessProxy.h:184:49

     0x00000001945ccf48:      add x29, sp, #0x60       ; =0x60 
     0x00000001945ccf4c:      mov x20, x1
     0x00000001945ccf50:      mov x19, x0
     0x00000001945ccf54:      ldr x8, [x19, #0xc0]
 ->  0x00000001945ccf58:     ldrb w8, [x8, #0x2d0]
     0x00000001945ccf5c:      cbz w8, 0x23d0a0         ; <+356> at WebPageProxy.cpp:5523
     0x00000001945ccf60:      add x8, sp, #0x8         ; =0x8 
     0x00000001945ccf64:      mov x0, x19
     0x00000001945ccf68:       bl 0x22e304             ; WebKit::WebPageProxy::currentURL at WebPageProxy.cpp:5503

[  0] 0x00000001945ccf58 WebKit`WebKit::WebPageProxy::processDidTerminate(WebKit::ProcessTerminationReason) + 28 at WebPageProxy.cpp:5515
[  1] 0x000000019464062b WebKit`WebKit::WebProcessProxy::didClose(IPC::Connection&) + 523 at WebProcessProxy.cpp:643:15
[  2] 0x000000019464062b WebKit`WebKit::WebProcessProxy::didClose(IPC::Connection&) + 523 at WebProcessProxy.cpp:643:15
[  3] 0x000000018c179dc7 JavaScriptCore`WTF::RunLoop::performWork() [inlined] WTF::Function<void ()>::operator()() const + 15 at Function.h:56:35
[  3] 0x000000018c179db8 JavaScriptCore`WTF::RunLoop::performWork() + 252 at RunLoop.cpp:106
[  4] 0x000000018c17a087 JavaScriptCore`WTF::RunLoop::performWork(void*) + 35 at RunLoopCF.cpp:38:37
[  5] 0x000000018435ca23 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 23 at CFRunLoop.c:1982:9
[  6] 0x000000018435c24b CoreFoundation`__CFRunLoopDoSources0 [inlined] __CFRunLoopDoSource0 + 67 at CFRunLoop.c:2017:13
[  6] 0x000000018435c208 CoreFoundation`__CFRunLoopDoSources0 + 208 at CFRunLoop.c:2053
[  7] 0x0000000184359dbb CoreFoundation`__CFRunLoopRun + 1203 at CFRunLoop.c:2920:41
[  8] 0x000000018427a5d7 CoreFoundation`CFRunLoopRunSpecific + 551 at CFRunLoop.c:3245:18
[  9] 0x000000018623b023 GraphicsServices`GSEventRunModal + 99 at GSEvent.c:2245:9
[ 10] 0x000000018e705a63 UIKit`UIApplicationMain + 235 at UIApplication.m:3956:5
[ 11] 0x000000010086e90b MobileSafari`main + 1927
[ 12] 0x0000000183d19faf libdyld.dylib`start + 3

[Chris Dumez]

<rdar://problem/36566237>

[Chris Dumez]

Created attachment 331559 [details]
Patch

[Brady Eidson]

"...no longer refs WebPageProxy objects..."

This means it used to.

Why did it stop doing so?

[Chris Dumez]

(In reply to Brady Eidson from comment #3)
> "...no longer refs WebPageProxy objects..."
> 
> This means it used to.
> 
> Why did it stop doing so?

Sorry, the explanation was only in the radar.

In WebProcessProxy::didClose():
Vector<RefPtr<WebPageProxy>> pages;
copyValuesToVector(m_pageMap, pages);

became (in r223149)
auto pages = copyToVector(m_pageMap.values());

“auto” here resolves to Vector<WebPageProxy*> so we no longer ref the pages. This is because m_pages is of type HashMap<uint64_t, WebPageProxy*>.

[Chris Dumez]

For the record, I tried writing an API test. The issue is that I cannot get the view to get destroyed in the processDidTerminate delegates. This is because all API tests run in an autorelease pool. Even though I release the webviews in the delegate, they only get deallocated later, at the end of the test, when the pool is drained.

[Chris Dumez]

Comment on attachment 331559 [details]
Patch

Clearing flags on attachment: 331559

Committed r227157: <https://trac.webkit.org/changeset/227157>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Did we audit all the other loops we changed to see if any others need this sort of fix to restore the old behavior?

[Chris Dumez]

(In reply to Darin Adler from comment #8)
> Did we audit all the other loops we changed to see if any others need this
> sort of fix to restore the old behavior?

Daniel Bates landed a follow-up patch in https://bugs.webkit.org/show_bug.cgi?id=178102.

[Daniel Bates]

(In reply to Chris Dumez from comment #9)
> (In reply to Darin Adler from comment #8)
> > Did we audit all the other loops we changed to see if any others need this
> > sort of fix to restore the old behavior?
> 
> Daniel Bates landed a follow-up patch in
> https://bugs.webkit.org/show_bug.cgi?id=178102.

I take it you meant to write that the follow up patch is on bug #181863.

[Chris Dumez]

(In reply to Daniel Bates from comment #10)
> (In reply to Chris Dumez from comment #9)
> > (In reply to Darin Adler from comment #8)
> > > Did we audit all the other loops we changed to see if any others need this
> > > sort of fix to restore the old behavior?
> > 
> > Daniel Bates landed a follow-up patch in
> > https://bugs.webkit.org/show_bug.cgi?id=178102.
> 
> I take it you meant to write that the follow up patch is on bug #181863.

Yes indeed.