|Summary:||HTML5 Database stops executing transactions if the URL hash changes while a transaction is open and an XHR is in progress.|
|Product:||WebKit||Reporter:||Andrew Grieve <agrieve>|
|Component:||New Bugs||Assignee:||Nobody <webkit-unassigned>|
|Severity:||Major||CC:||abarth, benm, darin|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
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
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
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