Bug 196588

Summary: The page's focusedFrame / frameSetLargestFrame do not get cleared on process swap or crash
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, commit-queue, darin, ggaren, Hironori.Fujii, rniwa, simon.fraser, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Chris Dumez 2019-04-03 19:18:37 PDT 
The page's focusedFrame / frameSetLargestFrame do not get cleared on process swap or crash.
Comment 1 Chris Dumez 2019-04-03 19:19:27 PDT 
<rdar://problem/49365787>
Comment 2 Chris Dumez 2019-04-03 19:22:11 PDT 
Created attachment 366693 [details]
Patch
Comment 3 Simon Fraser (smfr) 2019-04-03 20:08:46 PDT 
Comment on attachment 366693 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:6779
>      m_mainFrame = nullptr;
> +    m_focusedFrame = nullptr;
> +    m_frameSetLargestFrame = nullptr;

It would be nice to move all the data members which come from a specific web process into a struct, and clear the struct rather than doing it this more fragile way.
Comment 4 WebKit Commit Bot 2019-04-03 20:09:54 PDT 
Comment on attachment 366693 [details]
Patch

Clearing flags on attachment: 366693

Committed r243848: <https://trac.webkit.org/changeset/243848>
Comment 5 WebKit Commit Bot 2019-04-03 20:09:56 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 6 Darin Adler 2019-04-03 20:17:35 PDT 
Comment on attachment 366693 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKit/ReloadPageAfterCrash.cpp:141
> +TEST(WebKit, FrameSetLargestFramAfterCrash)

Fram
Comment 7 Chris Dumez 2019-04-03 20:23:34 PDT 
(In reply to Darin Adler from comment #6)
> Comment on attachment 366693 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366693&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKit/ReloadPageAfterCrash.cpp:141
> > +TEST(WebKit, FrameSetLargestFramAfterCrash)
> 
> Fram

Fixed in <https://trac.webkit.org/changeset/243849>, thank you.
Comment 8 Fujii Hironori 2019-04-03 22:19:36 PDT 
Windows port can't compile.

> ..\..\Tools\TestWebKitAPI\Tests\WebKit\ReloadPageAfterCrash.cpp(136): error C3861: 'kill': identifier not found

Should I replace 'kill' with WKPageTerminate?
Comment 9 Ryosuke Niwa 2019-04-03 22:38:44 PDT 
(In reply to Fujii Hironori from comment #8)
> Windows port can't compile.
> 
> > ..\..\Tools\TestWebKitAPI\Tests\WebKit\ReloadPageAfterCrash.cpp(136): error C3861: 'kill': identifier not found
> 
> Should I replace 'kill' with WKPageTerminate?

Yes!
Comment 10 Fujii Hironori 2019-04-03 22:45:15 PDT 
Committed r243858: <https://trac.webkit.org/changeset/243858>
Comment 11 Chris Dumez 2019-04-04 06:57:28 PDT 
(In reply to Fujii Hironori from comment #8)
> Windows port can't compile.
> 
> > ..\..\Tools\TestWebKitAPI\Tests\WebKit\ReloadPageAfterCrash.cpp(136): error C3861: 'kill': identifier not found
> 
> Should I replace 'kill' with WKPageTerminate?

I seem to remember the crash handler was not called with wkpageterminate and the test would time out, which is why I used kill.
Comment 12 Chris Dumez 2019-04-04 07:05:05 PDT 
(In reply to Chris Dumez from comment #11)
> (In reply to Fujii Hironori from comment #8)
> > Windows port can't compile.
> > 
> > > ..\..\Tools\TestWebKitAPI\Tests\WebKit\ReloadPageAfterCrash.cpp(136): error C3861: 'kill': identifier not found
> > 
> > Should I replace 'kill' with WKPageTerminate?
> 
> I seem to remember the crash handler was not called with wkpageterminate and
> the test would time out, which is why I used kill.

Look at the bots, your follow up change made the tests time out on all platforms... please fix.
Comment 13 Chris Dumez 2019-04-04 07:05:28 PDT 
(In reply to Ryosuke Niwa from comment #9)
> (In reply to Fujii Hironori from comment #8)
> > Windows port can't compile.
> > 
> > > ..\..\Tools\TestWebKitAPI\Tests\WebKit\ReloadPageAfterCrash.cpp(136): error C3861: 'kill': identifier not found
> > 
> > Should I replace 'kill' with WKPageTerminate?
> 
> Yes!

No :’(
Comment 14 Chris Dumez 2019-04-04 08:45:34 PDT 
Rolled out Fujii's r243858 in <https://trac.webkit.org/changeset/243870> and disabled the new tests on Windows instead for now.