[webkit-changes] cvs commit: WebKit/WebView.subproj WebFrame.m
Geoffrey
ggaren at opensource.apple.com
Fri Dec 30 12:42:16 PST 2005
ggaren 05/12/30 12:42:16
Modified: . ChangeLog
. ChangeLog
WebView.subproj WebFrame.m
Added: manual-tests onunload-form-submit-crash.html
Log:
WebCore:
Added test case for <rdar://problem/4268278> Submitting a form in onUnload event
handler causes crash in -[WebDataSource(WebPrivate) _commitIfReady:]
* manual-tests/onunload-form-submit-crash.html: Added.
WebKit:
Reviewed by Eric.
Manual testcase added:
WebCore/manual-tests/onunload-form-submit-crash.html
- Fixed <rdar://problem/4268278> Submitting a form in onUnload event
handler causes crash in -[WebDataSource(WebPrivate) _commitIfReady:]
The problem is that the form submission in the unload event kicks off
a new load in the midst of the load that caused the unload event to
fire in the first place, so the two loads stomp each other.
The solution is to cancel the first load and let the unload handler's
load win. (Firefox does the same.)
* WebView.subproj/WebFrame.m:
(-[WebFrame _transitionToCommitted:]): Moved call to -closeURL up
the call stack to _continueLoadRequest. (See below.) This has the
side-effect of always firing the unload event, even if the new
datasource never becomes committed, which seems like a good thing.
(-[WebFrame _continueLoadRequestAfterNavigationPolicy:formState:]):
Call -closeURL here, instead of in _transitionToCommitted, so that the
unload handler can fire before we initialize any part of the load.
Check provisionalDataSource for nil to discover if the unload event
kicked off its own load.
Cleared up some coments.
(-[WebFrame _detachFromParent]):
It turns out that if you close the window instead of just navigating
to a new page, you get an alternate assertion failure/crash because
the load kicked off by the unload event handler generates resource
loader callbacks after the associated WebFrame/WebView has disappeared.
The nifty solution here is just to reverse the order of calls to
-stopLoading and -closeURL, thus guaranteeing that -stopLoading has the
last word when you close a window.
Revision Changes Path
1.67 +8 -0 WebCore/ChangeLog
Index: ChangeLog
===================================================================
RCS file: /cvs/root/WebCore/ChangeLog,v
retrieving revision 1.66
retrieving revision 1.67
diff -u -r1.66 -r1.67
--- ChangeLog 30 Dec 2005 14:51:30 -0000 1.66
+++ ChangeLog 30 Dec 2005 20:42:12 -0000 1.67
@@ -1,3 +1,10 @@
+2005-12-29 Geoffrey Garen <ggaren at apple.com>
+
+ Added test case for <rdar://problem/4268278> Submitting a form in onUnload event
+ handler causes crash in -[WebDataSource(WebPrivate) _commitIfReady:]
+
+ * manual-tests/onunload-form-submit-crash.html: Added.
+
2005-12-30 Anders Carlsson <andersca at mac.com>
Reviewed by Eric.
@@ -195,6 +202,7 @@
* ksvg2/svg/SVGRectElementImpl.cpp:
(SVGRectElementImpl::toPathData): fixed round rect calculations
+>>>>>>> 1.66
2005-12-29 Mark Rowe <opendarwin.org at bdash.net.nz>
Reviewed by eseidel, ggaren, darin.
1.1 WebCore/manual-tests/onunload-form-submit-crash.html
Index: onunload-form-submit-crash.html
===================================================================
<html>
<body onUnload="document.myForm.submit()">
<a href="" onClick="location.href=location.href; return false;">Click here and see if Safari crashes.</a>
<p>Still with me? Now close the window and see if Safari crashes.</p>
<form name="myForm">
</form>
</body>
<html>
1.3436 +42 -0 WebKit/ChangeLog
Index: ChangeLog
===================================================================
RCS file: /cvs/root/WebKit/ChangeLog,v
retrieving revision 1.3435
retrieving revision 1.3436
diff -u -r1.3435 -r1.3436
--- ChangeLog 30 Dec 2005 07:05:47 -0000 1.3435
+++ ChangeLog 30 Dec 2005 20:42:12 -0000 1.3436
@@ -1,3 +1,45 @@
+2005-12-29 Geoffrey Garen <ggaren at apple.com>
+
+ Reviewed by Eric.
+
+ Manual testcase added:
+ WebCore/manual-tests/onunload-form-submit-crash.html
+
+ - Fixed <rdar://problem/4268278> Submitting a form in onUnload event
+ handler causes crash in -[WebDataSource(WebPrivate) _commitIfReady:]
+
+ The problem is that the form submission in the unload event kicks off
+ a new load in the midst of the load that caused the unload event to
+ fire in the first place, so the two loads stomp each other.
+
+ The solution is to cancel the first load and let the unload handler's
+ load win. (Firefox does the same.)
+
+ * WebView.subproj/WebFrame.m:
+ (-[WebFrame _transitionToCommitted:]): Moved call to -closeURL up
+ the call stack to _continueLoadRequest. (See below.) This has the
+ side-effect of always firing the unload event, even if the new
+ datasource never becomes committed, which seems like a good thing.
+
+ (-[WebFrame _continueLoadRequestAfterNavigationPolicy:formState:]):
+ Call -closeURL here, instead of in _transitionToCommitted, so that the
+ unload handler can fire before we initialize any part of the load.
+
+ Check provisionalDataSource for nil to discover if the unload event
+ kicked off its own load.
+
+ Cleared up some coments.
+
+ (-[WebFrame _detachFromParent]):
+ It turns out that if you close the window instead of just navigating
+ to a new page, you get an alternate assertion failure/crash because
+ the load kicked off by the unload event handler generates resource
+ loader callbacks after the associated WebFrame/WebView has disappeared.
+
+ The nifty solution here is just to reverse the order of calls to
+ -stopLoading and -closeURL, thus guaranteeing that -stopLoading has the
+ last word when you close a window.
+
2005-12-30 Mitz Pettel <opendarwin.org at mitzpettel.com>
Reviewed by Eric, committed by Maciej.
1.267 +26 -15 WebKit/WebView.subproj/WebFrame.m
Index: WebFrame.m
===================================================================
RCS file: /cvs/root/WebKit/WebView.subproj/WebFrame.m,v
retrieving revision 1.266
retrieving revision 1.267
diff -u -r1.266 -r1.267
--- WebFrame.m 30 Dec 2005 05:11:46 -0000 1.266
+++ WebFrame.m 30 Dec 2005 20:42:15 -0000 1.267
@@ -691,11 +691,10 @@
{
WebBridge *bridge = _private->bridge;
- [self stopLoading];
- [self _saveScrollPositionAndViewStateToItem:[_private currentItem]];
-
[bridge closeURL];
+ [self stopLoading];
+ [self _saveScrollPositionAndViewStateToItem:[_private currentItem]];
[self _detachChildren];
[_private setWebView:nil];
@@ -851,8 +850,6 @@
[_private setProvisionalItem:nil];
}
- [[self _bridge] closeURL];
-
// Set the committed data source on the frame.
[self _setDataSource:_private->provisionalDataSource];
@@ -2400,21 +2397,36 @@
WebHistoryItem *item = [_private provisionalItem];
// Two reasons we can't continue:
- // 1) navigation policy delegate said we can't so request is nil.
- // 2) before unload event handler caused an alert to come up and the user said cancel.
+ // 1) Navigation policy delegate said we can't so request is nil. A primary case of this
+ // is the user responding Cancel to the form repost nag sheet.
+ // 2) User responded Cancel to an alert popped up by the before unload event handler.
// The "before unload" event handler runs only for the main frame.
BOOL canContinue = request && ([[self webView] mainFrame] != self || [_private->bridge shouldClose]);
+ // The call to closeURL below invokes the unload event handler, which can execute arbitrary
+ // JavaScript. If the script initiates a new load, we need to cancel the requested load,
+ // or the two will stomp each other.
+
+ // Cache the load type of the current request, since _private->policyLoadType will change
+ // if the unload handler initiates a load. (We use this value to determine if we need to
+ // reset the back/forward cursor.)
+ WebFrameLoadType requestLoadType = _private->policyLoadType;
+ if (canContinue) {
+ [self stopLoading];
+ [[self _bridge] closeURL];
+
+ canContinue = [self provisionalDataSource] == nil;
+ }
+
if (!canContinue) {
[self _setPolicyDataSource:nil];
- // If the delegate punts on the navigation, we have the problem that we have optimistically moved
- // the b/f cursor already, so move it back. For sanity we only do this if the navigation of the
- // target frame or top-level frame is canceled. A primary case of this is the user responding
- // Cancel to the form repost nag sheet.
+ // If the navigation request came from the back/forward menu, and we punt on it, we have the
+ // problem that we have optimistically moved the b/f cursor already, so move it back. For sanity,
+ // we only do this when punting a navigation for the target frame or top-level frame.
if (([item isTargetItem] || [[self webView] mainFrame] == self)
- && (_private->policyLoadType == WebFrameLoadTypeForward
- || _private->policyLoadType == WebFrameLoadTypeBack
- || _private->policyLoadType == WebFrameLoadTypeIndexedBackForward))
+ && (requestLoadType == WebFrameLoadTypeForward
+ || requestLoadType == WebFrameLoadTypeBack
+ || requestLoadType == WebFrameLoadTypeIndexedBackForward))
[[[self webView] mainFrame] _resetBackForwardList];
return;
}
@@ -2422,7 +2434,6 @@
WebFrameLoadType loadType = _private->policyLoadType;
WebDataSource *dataSource = [_private->policyDataSource retain];
- [self stopLoading];
[self _setLoadType:loadType];
[self _setProvisionalDataSource:dataSource];
[dataSource release];
More information about the webkit-changes
mailing list