Bug 25710 - HTML5 Database stops executing transactions if the URL hash changes while a transaction is open and an XHR is in progress.
Summary: HTML5 Database stops executing transactions if the URL hash changes while a t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P1 Major
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-11 17:51 PDT by Andrew Grieve
Modified: 2009-06-05 07:00 PDT (History)
3 users (show)

See Also:


Attachments
Test harness for reproducing the bug. (4.38 KB, text/html)
2009-05-11 17:52 PDT, Andrew Grieve
no flags Details
Patch to not stop databases when navigating the history to the same document with a fragment (8.91 KB, patch)
2009-05-26 03:18 PDT, Ben Murdoch
no flags Details | Formatted Diff | Diff
Patch to not stop the databases if the navigation is to a hash fragment on the same document. (8.98 KB, patch)
2009-05-27 07:40 PDT, Ben Murdoch
no flags Details | Formatted Diff | Diff
New patch with argument rather than member variable. (12.46 KB, patch)
2009-06-01 12:24 PDT, Ben Murdoch
no flags Details | Formatted Diff | Diff
Replaces a couple of tabs with spaces in previous patch. (12.42 KB, patch)
2009-06-01 12:34 PDT, Ben Murdoch
abarth: review-
Details | Formatted Diff | Diff
Updates WebCore.base.exp to fix build bustage. (13.45 KB, patch)
2009-06-02 04:18 PDT, Ben Murdoch
no flags Details | Formatted Diff | Diff
Fix build bustage. (13.45 KB, patch)
2009-06-02 04:37 PDT, Ben Murdoch
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Grieve 2009-05-11 17:51:14 PDT
Refer to attached test case to reproduce the problem.
Comment 1 Andrew Grieve 2009-05-11 17:52:09 PDT
Created attachment 30211 [details]
Test harness for reproducing the bug.
Comment 2 Ben Murdoch 2009-05-26 03:18:05 PDT
Created attachment 30666 [details]
Patch to not stop databases when navigating the history to the same document with a fragment

This patch adds code to not stop the databases if we are navigating the history to a hash fragment on the same document and includes a layout test to verify. The test harness attached to this bug passes with this patch applied.
Comment 3 Ben Murdoch 2009-05-26 08:26:13 PDT
Comment on attachment 30666 [details]
Patch to not stop databases when navigating the history to the same document with a fragment

The layout test needs a little tweaking, will repost shortly.
Comment 4 Ben Murdoch 2009-05-27 07:40:52 PDT
Created attachment 30708 [details]
Patch to not stop the databases if the navigation is to a hash fragment on the same document.

New patch.
Comment 5 Darin Adler 2009-06-01 00:38:36 PDT
Comment on attachment 30708 [details]
Patch to not stop the databases if the navigation is to a hash fragment on the same document.

This change looks OK, but I'm disappointed about the way it uses state in the frame loader to make stopLoading not stop the databases. This seems a bit roundabout, making the already-fragile FrameLoader worse. I think that adding an argument to stopLoading would be better.
Comment 6 Ben Murdoch 2009-06-01 12:23:50 PDT
(In reply to comment #5)
> (From update of attachment 30708 [details] [review])
> This change looks OK, but I'm disappointed about the way it uses state in the
> frame loader to make stopLoading not stop the databases. This seems a bit
> roundabout, making the already-fragile FrameLoader worse. I think that adding
> an argument to stopLoading would be better.
> 

OK, a new patch is one the way.

Cheers, Ben
Comment 7 Ben Murdoch 2009-06-01 12:24:58 PDT
Created attachment 30839 [details]
New patch with argument rather than member variable.

New patch with an argument to the function rather than a member variable tracking state.
Comment 8 Ben Murdoch 2009-06-01 12:34:45 PDT
Created attachment 30840 [details]
Replaces a couple of tabs with spaces in previous patch.
Comment 9 Darin Adler 2009-06-01 15:36:15 PDT
Comment on attachment 30840 [details]
Replaces a couple of tabs with spaces in previous patch.

> +        void stopLoading(DatabasePolicy databasePolicy = DatabasePolicyStop);

Can just say DatabasePolicy = DatabasePolicyStop -- no need for an argument name.

r=me
Comment 10 Adam Barth 2009-06-02 00:43:22 PDT
Will land.
Comment 11 Adam Barth 2009-06-02 00:57:13 PDT
Sending        LayoutTests/ChangeLog
Adding         LayoutTests/storage/hash-change-with-xhr-expected.txt
Adding         LayoutTests/storage/hash-change-with-xhr.html
Sending        WebCore/ChangeLog
Sending        WebCore/loader/DocumentLoader.cpp
Sending        WebCore/loader/DocumentLoader.h
Sending        WebCore/loader/FrameLoader.cpp
Sending        WebCore/loader/FrameLoader.h
Sending        WebCore/loader/FrameLoaderTypes.h
Sending        WebCore/page/Page.cpp
Transmitting file data ..........
Committed revision 44351.
Comment 12 Adam Barth 2009-06-02 01:04:21 PDT
Patch applies but does not compile.
Comment 13 Adam Barth 2009-06-02 01:05:03 PDT
Will revert.
Comment 14 Ben Murdoch 2009-06-02 04:17:32 PDT
(In reply to comment #12)
> Patch applies but does not compile.
> 

Ah, sorry. It was fine on Windows but needed to change the WebCore exports file on Mac to reflect the new method signatures. Fixed with the new patch.
Comment 15 Ben Murdoch 2009-06-02 04:18:43 PDT
Created attachment 30862 [details]
Updates WebCore.base.exp to fix build bustage.
Comment 16 Ben Murdoch 2009-06-02 04:37:00 PDT
Created attachment 30864 [details]
Fix build bustage.

Updates the WebCore exports.
Comment 17 Alexey Proskuryakov 2009-06-04 07:35:40 PDT
Comment on attachment 30864 [details]
Fix build bustage.

Based on Darin's earlier review, this look good for landing

+        * loader/DocumentLoader.cpp:
+        (WebCore::DocumentLoader::stopLoading):
+        * loader/DocumentLoader.h:
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::stopLoading):
+        (WebCore::FrameLoader::stopAllLoaders):
+        * loader/FrameLoader.h:
+        * loader/FrameLoaderTypes.h:
+        (WebCore::):
+        * page/Page.cpp:
+        (WebCore::Page::goToItem):
+        * WebCore.base.exp:

We much prefer detailed per-function comments in ChangeLogs.
Comment 18 Oliver Hunt 2009-06-05 07:00:36 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/storage/hash-change-with-xhr-expected.txt
	A	LayoutTests/storage/hash-change-with-xhr.html
	M	WebCore/ChangeLog
	M	WebCore/WebCore.base.exp
	M	WebCore/loader/DocumentLoader.cpp
	M	WebCore/loader/DocumentLoader.h
	M	WebCore/loader/FrameLoader.cpp
	M	WebCore/loader/FrameLoader.h
	M	WebCore/loader/FrameLoaderTypes.h
	M	WebCore/page/Page.cpp
Committed r44468