Bug 25710 – HTML5 Database stops executing transactions if the URL hash changes while a transaction is open and an XHR is in progress.

• Component: New Bugs
• Status: RESOLVED FIXED

[Andrew Grieve]

Refer to attached test case to reproduce the problem.

[Andrew Grieve]

Created attachment 30211 [details]
Test harness for reproducing the bug.

[Ben Murdoch]

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.

[Ben Murdoch]

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.

[Ben Murdoch]

Created attachment 30708 [details]
Patch to not stop the databases if the navigation is to a hash fragment on the same document.

New patch.

[Darin Adler]

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.

[Ben Murdoch]

(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

[Ben Murdoch]

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.

[Ben Murdoch]

Created attachment 30840 [details]
Replaces a couple of tabs with spaces in previous patch.

[Darin Adler]

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

[Adam Barth]

Will land.

[Adam Barth]

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.

[Adam Barth]

Patch applies but does not compile.

[Adam Barth]

Will revert.

[Ben Murdoch]

(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.

[Ben Murdoch]

Created attachment 30862 [details]
Updates WebCore.base.exp to fix build bustage.

[Ben Murdoch]

Created attachment 30864 [details]
Fix build bustage.

Updates the WebCore exports.

[Alexey Proskuryakov]

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.

[Oliver Hunt]

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