Bug 156938

Summary: Tie the DiagnosticLoggingClient's lifetime to the Page
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, kling, koivisto, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2016-04-22 16:43:44 PDT
Tie the DiagnosticLoggingClient's lifetime to the Page rather than to the MainFrame. The diagnostic logging client in WebKit2 requires the WebPage to be alive in order to send IPC to the UIProcess. The WebPage owns the Page and Page is not refCounted so the lifetime of the DiagnosticLoggingClient should now be tied to the one of the WebPage as well.

Previously, DiagnosticLoggingClient would stay alive as long as the MainFrame and could apparently in rare cases outlive the WebPage, thus crashing when trying to send the IPC (rdar://problem/25851499).
Comment 1 Chris Dumez 2016-04-22 16:44:11 PDT
rdar://problem/25851499
Comment 2 Chris Dumez 2016-04-22 17:03:08 PDT
Created attachment 277116 [details]
Patch
Comment 3 Chris Dumez 2016-04-22 18:20:31 PDT
Created attachment 277123 [details]
Patch
Comment 4 Chris Dumez 2016-04-22 18:23:03 PDT
Created attachment 277124 [details]
Patch
Comment 5 Antti Koivisto 2016-04-22 23:32:38 PDT
Comment on attachment 277124 [details]
Patch

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

> Source/WebCore/page/Page.cpp:287
> +    if (m_diagnosticLoggingClient)
> +        m_diagnosticLoggingClient->pageDestroyed();

Why this instead of using std::unique_ptr for ownership?
Comment 6 Antti Koivisto 2016-04-22 23:36:59 PDT
Comment on attachment 277124 [details]
Patch

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

>> Source/WebCore/page/Page.cpp:287
>> +        m_diagnosticLoggingClient->pageDestroyed();
> 
> Why this instead of using std::unique_ptr for ownership?

Is there a chance that someone might access the dangling pointer during the rest of the destructor? Maybe null it?
Comment 7 Chris Dumez 2016-04-23 13:44:50 PDT
Created attachment 277163 [details]
Patch
Comment 8 WebKit Commit Bot 2016-04-23 14:33:28 PDT
Comment on attachment 277163 [details]
Patch

Clearing flags on attachment: 277163

Committed r199955: <http://trac.webkit.org/changeset/199955>
Comment 9 WebKit Commit Bot 2016-04-23 14:33:33 PDT
All reviewed patches have been landed.  Closing bug.