Summary: | [WK2] Page reloading will crash UIProcess after WebProcess was killed | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adenilson Cavalcanti Silva <savagobr> | ||||||||||||||||||||||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | andersca, buildbot, cdumez, csaavedra, gyuyoung.kim, rakuco, rniwa, sam, savagobr, simon.fraser, webkit.review.bot, zan | ||||||||||||||||||||||||||
Priority: | P1 | ||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Adenilson Cavalcanti Silva
2013-02-08 08:49:55 PST
Created attachment 187328 [details]
The backtrace
The backtrace.
Created attachment 187331 [details]
Move the place where the object is created, add a test for pointer state in initializeWebPage()
Comment on attachment 187331 [details]
Move the place where the object is created, add a test for pointer state in initializeWebPage()
Are you sure ENABLE(INSPECTOR) and ENABLE(INSPECTOR_SERVER) are equivalent? Note you've moved something from one protector to the other.
Rafael Probably they are different. But if you look at the code, the calls to WebInspectorProxy methods only happen on ENABLE(INSPECTOR_SERVER) blocks. Those are the calls that can result in crashes if the pointer are set to null. A heads up on the bug: I started to write a WK2 API to stress the scenario (crash/reload). It is building/running on mac and EFL ports (GTK should be next). For the record: the same bug will make EFL Minibrowser crash, if the WebProcess was killed. Created attachment 187438 [details]
EFL backtrace
*** Bug 109407 has been marked as a duplicate of this bug. *** I have just been made aware of this bug report. I already had a patch for this as well at Bug 109407. My patch seems a bit simpler. Comment on attachment 187331 [details] Move the place where the object is created, add a test for pointer state in initializeWebPage() View in context: https://bugs.webkit.org/attachment.cgi?id=187331&action=review > Source/WebKit2/UIProcess/WebPageProxy.cpp:436 > + if (!m_inspector) If a port has INSPECTOR enabled but INSPECTOR_SERVER disabled, it is likely going to cause problems as m_inspector is not going to be initialized. Please check my patch at Bug 109407. (In reply to comment #9) > I have just been made aware of this bug report. I already had a patch for this as well at Bug 109407. > My patch seems a bit simpler. The patch in that bug seems like the right fix. Christophe, can you please post your patch for review here? Created attachment 187617 [details]
Alternative patch
Created attachment 187630 [details]
Following approach suggested by Dumez, plus test.
Comment on attachment 187630 [details] Following approach suggested by Dumez, plus test. View in context: https://bugs.webkit.org/attachment.cgi?id=187630&action=review > Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:31 > +#include <stdio.h> Is this really needed? > Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:35 > +static bool done = false; I think we only need one boolean. > Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:40 > + // Ok, this is the first load iirc. Coding style says comments should end with a period. > Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:43 > + EXPECT_TRUE(done); This check right after assigning does not seem useful. > Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:48 > + EXPECT_EQ(static_cast<uint32_t>(kWKFrameLoadStateFinished), WKFrameGetFrameLoadState(frame)); This check could be done in CrashReload test function. > Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:50 > + EXPECT_TRUE(loaded); This check right after assigning does not seem useful. > Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:60 > + loaderClient.didFinishLoadForFrame = didFinishLoad; Should we make sure processDidCrash is called as well? > Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:67 > + Util::sleep(3); I don't understand why we need to sleep. > Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:69 > + Util::sleep(3); Ditto. > Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:70 > + WKPageLoadURL(webView.page(), adoptWK(WKURLCreateWithUTF8CString("about:blank")).get()); Since we use the same WKUrl twice, maybe we could create it once and store it in a variable? Christophe Thanks for reviewing, I'm working in a new patch addressing the issues (plus adding support for GTK test building). Comment on attachment 187630 [details] Following approach suggested by Dumez, plus test. Attachment 187630 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16504235 Created attachment 187686 [details]
Adding test to GTK buildsystem, addressing reviewer's comments
Looking into PlatformUtilities.h, I've found this: // Runs a platform runloop until the 'done' is true. void run(bool* done); So I think we will need two control variables instead of a single one. Created attachment 187718 [details]
Now it should execute reload properly.
Comment on attachment 187718 [details] Now it should execute reload properly. Attachment 187718 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16486633 Created attachment 187759 [details]
trying to make xcode happy again.
Comment on attachment 187759 [details] trying to make xcode happy again. Attachment 187759 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16486734 Created attachment 187767 [details]
Xcode try again #001
Created attachment 187768 [details]
patch #01
Comment on attachment 187768 [details] patch #01 View in context: https://bugs.webkit.org/attachment.cgi?id=187768&action=review Some comments: > Source/WebKit2/ChangeLog:9 > + Re-initialize pointer to WebInspectorProxy object before calling > + initializeWebPage(). You should also explain why you make the change, not only what the change is. > Tools/ChangeLog:14 > + * TestWebKitAPI/Tests/WebKit2/CrashReload.cpp: Added. I'd argue the name CrashReload is a little too generic. It is hard to pinpoint what is tested in there. > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:1091 > 2E7765CD16C4D80A00BA2BB1 /* mainIOS.mm in Sources */, > 2E7765CF16C4D81100BA2BB1 /* mainMac.mm in Sources */, > + 8A3AF93B16C9ED2700D248C1 /* CrashReload.cpp in Sources */, The build section should be sorted alphabetically > Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:35 > +static bool first = false; > +static bool second = false; Those names are too generic. First, could they be made local instead of being global? Second, they should be named in a way take make their purpose obvious. > Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:39 > + // Ok, this is the first load. Comments like this are not helping. > Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:43 > + if (!first) { > + first = true; > + return; > + } Should't this also check second is false? If second is true here > Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:45 > + // Second time is the charm. Comments like this are not helping, comments should explain the "Why" of the code. > Tools/TestWebKitAPI/Tests/WebKit2/CrashReload.cpp:74 > + // Load a blank page and next kills it Period at the end of the sentence. Created attachment 187968 [details]
Addressing reviewers comments: Changelog, comments, variable names, test name, xcode files ordering.
Comment on attachment 187968 [details] Addressing reviewers comments: Changelog, comments, variable names, test name, xcode files ordering. View in context: https://bugs.webkit.org/attachment.cgi?id=187968&action=review > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:-1087 > - 2E7765CD16C4D80A00BA2BB1 /* mainIOS.mm in Sources */, > - 2E7765CF16C4D81100BA2BB1 /* mainMac.mm in Sources */, Where is mainMac.mm? Created attachment 187975 [details]
Should have the xcode changes right (hopefully)
Comment on attachment 187617 [details]
Alternative patch
Clearing this, patch with test available for landing.
Comment on attachment 187975 [details] Should have the xcode changes right (hopefully) Clearing flags on attachment: 187975 Committed r142704: <http://trac.webkit.org/changeset/142704> All reviewed patches have been landed. Closing bug. *** Bug 109670 has been marked as a duplicate of this bug. *** |