Bug 181771 - REGRESSION (r223149): WebProcessProxy::didClose() no longer refs WebPageProxy objects
Summary: REGRESSION (r223149): WebProcessProxy::didClose() no longer refs WebPageProxy...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar, Regression
Depends on:
Blocks: 178102
  Show dependency treegraph
 
Reported: 2018-01-17 16:36 PST by Chris Dumez
Modified: 2018-01-22 12:20 PST (History)
7 users (show)

See Also:


Attachments
Patch (1.39 KB, patch)
2018-01-17 16:41 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2018-01-17 16:36:49 PST
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
Comment 1 Chris Dumez 2018-01-17 16:37:02 PST
<rdar://problem/36566237>
Comment 2 Chris Dumez 2018-01-17 16:41:15 PST
Created attachment 331559 [details]
Patch
Comment 3 Brady Eidson 2018-01-17 18:41:39 PST
"...no longer refs WebPageProxy objects..."

This means it used to.

Why did it stop doing so?
Comment 4 Chris Dumez 2018-01-17 19:01:43 PST
(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*>.
Comment 5 Chris Dumez 2018-01-18 10:19:28 PST
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.
Comment 6 Chris Dumez 2018-01-18 12:11:03 PST
Comment on attachment 331559 [details]
Patch

Clearing flags on attachment: 331559

Committed r227157: <https://trac.webkit.org/changeset/227157>
Comment 7 Chris Dumez 2018-01-18 12:11:05 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Darin Adler 2018-01-21 23:40:32 PST
Did we audit all the other loops we changed to see if any others need this sort of fix to restore the old behavior?
Comment 9 Chris Dumez 2018-01-22 09:30:38 PST
(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.
Comment 10 Daniel Bates 2018-01-22 11:24:16 PST
(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.
Comment 11 Chris Dumez 2018-01-22 12:20:08 PST
(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.