Bug 208918

Summary: FrameLoader should own its FrameLoaderClient
Product: WebKit Reporter: youenn fablet <youennf>
Component: Page LoadingAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, cdumez, commit-queue, ews-watchlist, ggaren, japhet, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 209179    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch for landing none

Description youenn fablet 2020-03-11 07:24:29 PDT 
FrameLoader should own its FrameLoaderClient
Comment 1 youenn fablet 2020-03-11 07:31:23 PDT 
Created attachment 393238 [details]
Patch
Comment 2 youenn fablet 2020-03-11 09:40:23 PDT 
Created attachment 393254 [details]
Patch
Comment 3 youenn fablet 2020-03-11 10:29:37 PDT 
Comment on attachment 393254 [details]
Patch

Need to fix win port. Putting under review in the meantime.
Comment 4 youenn fablet 2020-03-12 03:13:44 PDT 
Created attachment 393353 [details]
Patch
Comment 5 youenn fablet 2020-03-12 04:48:12 PDT 
Created attachment 393358 [details]
Patch
Comment 6 youenn fablet 2020-03-12 05:52:32 PDT 
Created attachment 393363 [details]
Patch
Comment 7 Geoffrey Garen 2020-03-12 08:58:47 PDT 
Comment on attachment 393363 [details]
Patch

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

r=me

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:-175
> -void WebFrameLoaderClient::frameLoaderDestroyed()
> -{
> -    m_frame->invalidate();
> -
> -    // Balances explicit ref() in WebFrame::create().
> -    m_frame->deref();
> -}

It is a joy to see code like this disappear 🥳
Comment 8 WebKit Commit Bot 2020-03-12 10:56:14 PDT 
Comment on attachment 393363 [details]
Patch

Clearing flags on attachment: 393363

Committed r258339: <https://trac.webkit.org/changeset/258339>
Comment 9 WebKit Commit Bot 2020-03-12 10:56:16 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2020-03-12 10:57:17 PDT 
<rdar://problem/60377728>
Comment 11 WebKit Commit Bot 2020-03-17 08:31:21 PDT 
Re-opened since this is blocked by bug 209179
Comment 12 youenn fablet 2020-03-18 05:53:35 PDT 
Created attachment 393840 [details]
Patch for landing
Comment 13 youenn fablet 2020-03-18 05:55:41 PDT 
(In reply to youenn fablet from comment #12)
> Created attachment 393840 [details]
> Patch for landing

Fixed rendering issue by delaying setting WebPage::m_mainFrame to exactly what was done before. The issue was that we were unfreezing too quickly in case of ProcessSwapping.
This does not fix the potential raciness in the two DrawingArea messages of the being-swapped WebPageProxy, but this patch does not make it worse.
Comment 14 youenn fablet 2020-03-18 05:58:20 PDT 
Created attachment 393842 [details]
Patch for landing
Comment 15 WebKit Commit Bot 2020-03-18 06:59:28 PDT 
Comment on attachment 393842 [details]
Patch for landing

Clearing flags on attachment: 393842

Committed r258628: <https://trac.webkit.org/changeset/258628>
Comment 16 WebKit Commit Bot 2020-03-18 06:59:30 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 17 Geoffrey Garen 2020-03-18 07:57:01 PDT 
Can you file a follow up bug about the DrawingAreaProxy race — probably for Chris or Tim? Sounds like something worth fixing.
Comment 18 youenn fablet 2020-03-18 08:00:30 PDT 
(In reply to Geoffrey Garen from comment #17)
> Can you file a follow up bug about the DrawingAreaProxy race — probably for
> Chris or Tim? Sounds like something worth fixing.

Sure.
To be clear, the race is hopefully probably theoretical only.
My main issue is that the design seems a bit weak since we keep two IPC listeners from two different processes but for the same page.

I'll do some follow-up clean-up related to WebPage.
Comment 19 youenn fablet 2020-03-18 08:07:07 PDT 
https://bugs.webkit.org/show_bug.cgi?id=209232