Bug 196588 - The page's focusedFrame / frameSetLargestFrame do not get cleared on process swap or crash
Summary: The page's focusedFrame / frameSetLargestFrame do not get cleared on process ...
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
Depends on:
Blocks:
 
Reported: 2019-04-03 19:18 PDT by Chris Dumez
Modified: 2019-04-04 08:45 PDT (History)
10 users (show)

See Also:


Attachments
Patch (5.87 KB, patch)
2019-04-03 19:22 PDT, 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 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.