Bug 79206 - REGRESSION(r106388): Form state is restored to a wrong document
Summary: REGRESSION(r106388): Form state is restored to a wrong document
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-22 00:46 PST by Kent Tamura
Modified: 2012-03-01 18:30 PST (History)
10 users (show)

See Also:


Attachments
Patch (5.64 KB, patch)
2012-02-22 00:52 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (5.62 KB, patch)
2012-02-22 18:03 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 3 (5.79 KB, patch)
2012-02-26 23:16 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 4 (7.85 KB, patch)
2012-02-28 21:29 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 5 (7.77 KB, patch)
2012-02-29 22:19 PST, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2012-02-22 00:46:11 PST
Note: r106388 is not a root cause of this bug, but it unveiled the bug.

When a document is loaded with FrameLoadTypeRedirectWithLockedBackForwardList, the URL of a HistoryItem can be mismatched with the URL of the actual document. So form state can be restored to a wrong document.
We don't save the form state if a form control is not changed. r106388 changed this behavior for type=hidden.
Comment 1 Kent Tamura 2012-02-22 00:52:14 PST
Created attachment 128149 [details]
Patch
Comment 2 Dimitri Glazkov (Google) 2012-02-22 09:18:07 PST
Comment on attachment 128149 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128149&action=review

> Source/WebCore/loader/HistoryController.cpp:203
> +    if (itemToRestore->urlString() == doc->url().string()) {

Are we supposed to just compare URLs as strings like this?
Comment 3 Alexey Proskuryakov 2012-02-22 10:03:12 PST
Comment on attachment 128149 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128149&action=review

> Source/WebCore/ChangeLog:11
> +        When a document is loaded with FrameLoadTypeRedirectWithLockedBackForwardList,
> +        the URL of a HistoryItem can be mismatched with the URL of the
> +        actual document. It was possible to store form state to a wrong
> +        document.

Is it expected that it can be mismatched?

It sounds like there is already a check for this somewhere that is giving a wrong answer, and this patch adds another check on top of it. Can the original check be fixed instead?
Comment 4 Kent Tamura 2012-02-22 18:01:04 PST
Comment on attachment 128149 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128149&action=review

>> Source/WebCore/ChangeLog:11
>> +        document.
> 
> Is it expected that it can be mismatched?
> 
> It sounds like there is already a check for this somewhere that is giving a wrong answer, and this patch adds another check on top of it. Can the original check be fixed instead?

I'm not familiar with loader/history code, but my understanding is that it's expected.

When this bug happens, HistoryController::restoreDocumentState() is called through DocumentWriter::createDocument() -> FrameLoader::didBeginDocument().  I think HistoryController::restoreDOcumentState() is the best place to check this if the assumption of the URL mismatch is correct.

>> Source/WebCore/loader/HistoryController.cpp:203
>> +    if (itemToRestore->urlString() == doc->url().string()) {
> 
> Are we supposed to just compare URLs as strings like this?

I'll change this to KURL comparison.
Comment 5 Kent Tamura 2012-02-22 18:03:38 PST
Created attachment 128354 [details]
Patch 2

string -> KURL
Comment 6 Kent Tamura 2012-02-23 17:56:26 PST
I need comments from loader/history experts.
Adam, Antti, Brady, Mihai?
Comment 7 Adam Barth 2012-02-23 23:10:18 PST
Comment on attachment 128354 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=128354&action=review

> Source/WebCore/loader/HistoryController.cpp:203
> +    if (itemToRestore->url() == doc->url()) {

This doesn't seem right.  What if I load two documents sequentially with the same URL?  The history system confuses me.
Comment 8 Brady Eidson 2012-02-23 23:17:52 PST
(In reply to comment #7)
> (From update of attachment 128354 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128354&action=review
> 
> > Source/WebCore/loader/HistoryController.cpp:203
> > +    if (itemToRestore->url() == doc->url()) {
> 
> This doesn't seem right.  What if I load two documents sequentially with the same URL?  The history system confuses me.

This problem has come up before in a few other places, though I can't recall any right now.  A solution I think is reasonable:

We have HistoryItems tagged with unique IDs.  Perhaps we could start tagging Documents with a unique ID and if a Document is created as a result of back/forward navigation, it could adopt the unique ID of its associated HistoryItem.

Then the form value restoration could be comparing the HistoryItem's id to the Document's ID.
Comment 9 Adam Barth 2012-02-23 23:40:19 PST
Don't we already have a sequence number for documents?
Comment 10 Kent Tamura 2012-02-24 00:12:36 PST
(In reply to comment #8)
> > This doesn't seem right.  What if I load two documents sequentially with the same URL?  The history system confuses me.
> 
> This problem has come up before in a few other places, though I can't recall any right now.  A solution I think is reasonable:
> 
> We have HistoryItems tagged with unique IDs.  Perhaps we could start tagging Documents with a unique ID and if a Document is created as a result of back/forward navigation, it could adopt the unique ID of its associated HistoryItem.
> 
> Then the form value restoration could be comparing the HistoryItem's id to the Document's ID.

Adding RefPtr<HistoryItem> member to Document would be enough.  The member is set at somewhere inside of FrameLoader::loadItem(), right?

(In reply to comment #9)
> Don't we already have a sequence number for documents?

We have m_docID.  However I think we can't use it because of SameDocumentNavigation.
Comment 11 Brady Eidson 2012-02-24 09:04:23 PST
(In reply to comment #10)
> (In reply to comment #8)
> > > This doesn't seem right.  What if I load two documents sequentially with the same URL?  The history system confuses me.
> > 
> > This problem has come up before in a few other places, though I can't recall any right now.  A solution I think is reasonable:
> > 
> > We have HistoryItems tagged with unique IDs.  Perhaps we could start tagging Documents with a unique ID and if a Document is created as a result of back/forward navigation, it could adopt the unique ID of its associated HistoryItem.
> > 
> > Then the form value restoration could be comparing the HistoryItem's id to the Document's ID.
> 

> (In reply to comment #9)
> > Don't we already have a sequence number for documents?
> 
> We have m_docID.  However I think we can't use it because of SameDocumentNavigation.

Yup, totally remember the document id existing for this reason!

> Adding RefPtr<HistoryItem> member to Document would be enough.  The member is set at somewhere inside of FrameLoader::loadItem(), right?

This sounds fine to me.  I'm not 100% sure FrameLoader::loadItem() is exactly right but I don't have code in front of me.  It should basically be wherever the Document is actually constructed, or the closest point to that where it's practical during back/forward navigation.
Comment 12 Kent Tamura 2012-02-26 23:16:13 PST
Created attachment 128961 [details]
Patch 3

HistoryItem::isCurrentDocument
Comment 13 Kent Tamura 2012-02-26 23:28:06 PST
(In reply to comment #11)
> > Adding RefPtr<HistoryItem> member to Document would be enough.  The member is set at somewhere inside of FrameLoader::loadItem(), right?
> 
> This sounds fine to me.  I'm not 100% sure FrameLoader::loadItem() is exactly right but I don't have code in front of me.  It should basically be wherever the Document is actually constructed, or the closest point to that where it's practical during back/forward navigation.

I tried to add RefPtr<HistoryItem>, and failed.
FrameLoader::loadItem() was not used in this case.
I needed to pass the current HistoryItem to a document if the loadType() was not FrameLoadTypeRedirectWithLockedBackForwardList, or the URL was different from the current HistoryItem, however I felt it was nonsense because HistoryController::restoreDocumentState() already has switch-case for loadType().

I found HistoryController::saveDocumentState() used HisotryItem::isCurrentDocument().  Using it in HistoryController::restoreDocumentState() is consistent and reasonable.
Comment 14 Brady Eidson 2012-02-27 08:55:29 PST
I would like to review this with code in front of me before it's landed but won't be at my work machine for a few hours.  So in case someone else reviews in the next few hours please hold off landing.
Comment 15 Brady Eidson 2012-02-27 10:56:51 PST
Comment on attachment 128961 [details]
Patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=128961&action=review

> Source/WebCore/loader/HistoryController.cpp:206
> +    if (itemToRestore->isCurrentDocument(doc)) {
> +        LOG(Loading, "WebCoreLoading %s: restoring form state from %p", m_frame->tree()->uniqueName().string().utf8().data(), itemToRestore);
> +        doc->setStateForNewFormElements(itemToRestore->documentState());
> +    }

Doesn't this code do the exact same wrong thing Adam and I alluded to? 

(HistoryItem::isCurrentDocument() just does a URL compare)
Comment 16 Kent Tamura 2012-02-27 22:22:48 PST
(In reply to comment #15)
> (From update of attachment 128961 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128961&action=review
> 
> > Source/WebCore/loader/HistoryController.cpp:206
> > +    if (itemToRestore->isCurrentDocument(doc)) {
> > +        LOG(Loading, "WebCoreLoading %s: restoring form state from %p", m_frame->tree()->uniqueName().string().utf8().data(), itemToRestore);
> > +        doc->setStateForNewFormElements(itemToRestore->documentState());
> > +    }
> 
> Doesn't this code do the exact same wrong thing Adam and I alluded to? 
> 
> (HistoryItem::isCurrentDocument() just does a URL compare)

Yes, this is almost equivalent to my "Patch 2".

I think adding an ID to Document in order to identify the originator HistoryItem doesn't work.
* In LayoutTests/fast/history/saves-satet-after-fragment-nav.html, we expect restoring a form state even if a document is loaded with a different HistoryItem.
* We have history.replaceState(), so we need to check URL equality anyway.
Comment 17 Brady Eidson 2012-02-27 23:13:10 PST
(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 128961 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=128961&action=review
> > 
> > > Source/WebCore/loader/HistoryController.cpp:206
> > > +    if (itemToRestore->isCurrentDocument(doc)) {
> > > +        LOG(Loading, "WebCoreLoading %s: restoring form state from %p", m_frame->tree()->uniqueName().string().utf8().data(), itemToRestore);
> > > +        doc->setStateForNewFormElements(itemToRestore->documentState());
> > > +    }
> > 
> > Doesn't this code do the exact same wrong thing Adam and I alluded to? 
> > 
> > (HistoryItem::isCurrentDocument() just does a URL compare)
> 
> Yes, this is almost equivalent to my "Patch 2".

Maybe we're on different wavelengths here, maybe you can try to help me understand...

> I think adding an ID to Document in order to identify the originator HistoryItem doesn't work.
> * In LayoutTests/fast/history/saves-satet-after-fragment-nav.html, we expect restoring a form state even if a document is loaded with a different HistoryItem.

Isn't this whole patch about loading a new document with a different HistoryItem?  And that's the behavior we're trying to get right?

> * We have history.replaceState(), so we need to check URL equality anyway.

history.replaceState() doesn't change the unique identity of the document but *does* change the URL...  but it only affects the current document anyway so I'm not sure why it's relevant.

Plus none of this discussion has touched on the concern of what happens if you get the exact same URL in the history list twice in a row; which is actually quite possible with replaceState or pushState.  


(Also, this discussion has raised the gnarly fact that the HTML5 history state APIs have a name collision with what we previously called "document state" as attached to our history items, and we should probably rename to reduce that confusion.)
Comment 18 Kent Tamura 2012-02-28 02:37:51 PST
(In reply to comment #17)

I'm sorry for my confusing comment.  I tried to fix HistoryItem::isCurrentDocument(), and had some extra issues.

The fix of this bug is:
  A form state in a HistoryItem should be restored only if the document is directly loaded from the HistoryItem. (not redirection)

I have no good idea of a way to distinguish a direct load and a redirect load in DocumentWriter::begin(), in which we create a Document.  Would you make some implementation advice please?
Comment 19 Brady Eidson 2012-02-28 13:50:48 PST
(In reply to comment #18)
> (In reply to comment #17)
> 
> I'm sorry for my confusing comment.  I tried to fix HistoryItem::isCurrentDocument(), and had some extra issues.
> 
> The fix of this bug is:
>   A form state in a HistoryItem should be restored only if the document is directly loaded from the HistoryItem. (not redirection)
> 
> I have no good idea of a way to distinguish a direct load and a redirect load in DocumentWriter::begin(), in which we create a Document.  Would you make some implementation advice please?

Why not create the Document with it's originating history item, but then the first time the document is redirected clear the item?

Then, when it's time to restore forms:
-If you have the item to restore from, you know it's a match.
-If you have no item, you know you were redirected and shouldn't restore the forms.
Comment 20 Kent Tamura 2012-02-28 21:29:46 PST
Created attachment 129382 [details]
Patch 4
Comment 21 Kent Tamura 2012-02-28 21:33:29 PST
(In reply to comment #20)
> Created an attachment (id=129382) [details]
> Patch 4

I tried to add new member to Document, but I realized loaders knew necessary information, and stop adding new member to Document.
Comment 22 Brady Eidson 2012-02-29 10:09:21 PST
Comment on attachment 129382 [details]
Patch 4

View in context: https://bugs.webkit.org/attachment.cgi?id=129382&action=review

I like this approach!

But have a few problems with this patch:

> Source/WebCore/loader/FrameLoader.cpp:3174
> +    m_requestedHistoryItem = item;

m_requestedHistoryItem is set in ::loadItem(), but never cleared unless a new loadItem takes place.

Nominally this doesn't seem so bad - Just an extra HistoryItem hanging around that will be destroyed when the FrameLoader is.

But HistoryItems can be surprisingly heavyweight objects, and I think we should clear it out once we don't need it anymore.  Perhaps as the very last part of didFinishLoad?

> Source/WebCore/loader/FrameLoader.cpp:3187
> +bool FrameLoader::didLoadWithLoadItem() const
> +{
> +    return m_requestedHistoryItem.get() == history()->currentItem();
> +}

This method name makes me cringe.  Plus, what the method does seems unnecessarily specific to this problem.

I would suggest one of two things:
1 - Make it an accessor for the requestedItem, and have HistoryController do the specific comparison it needs.
2 - Rename it "isRequestedItemCurrent" or "wasCurrentItemRequested"

In fact, I hate both of those names too...  I'd personally do number 1...  ;)
Comment 23 Kent Tamura 2012-02-29 22:19:10 PST
Created attachment 129632 [details]
Patch 5
Comment 24 Kent Tamura 2012-02-29 22:21:41 PST
Comment on attachment 129382 [details]
Patch 4

View in context: https://bugs.webkit.org/attachment.cgi?id=129382&action=review

Thank you for the comments.

>> Source/WebCore/loader/FrameLoader.cpp:3174
>> +    m_requestedHistoryItem = item;
> 
> m_requestedHistoryItem is set in ::loadItem(), but never cleared unless a new loadItem takes place.
> 
> Nominally this doesn't seem so bad - Just an extra HistoryItem hanging around that will be destroyed when the FrameLoader is.
> 
> But HistoryItems can be surprisingly heavyweight objects, and I think we should clear it out once we don't need it anymore.  Perhaps as the very last part of didFinishLoad?

didFinishLoad() of what class?
I added code to clear the HistoryItem to a place where m_isComplete is set to true.

>> Source/WebCore/loader/FrameLoader.cpp:3187
>> +}
> 
> This method name makes me cringe.  Plus, what the method does seems unnecessarily specific to this problem.
> 
> I would suggest one of two things:
> 1 - Make it an accessor for the requestedItem, and have HistoryController do the specific comparison it needs.
> 2 - Rename it "isRequestedItemCurrent" or "wasCurrentItemRequested"
> 
> In fact, I hate both of those names too...  I'd personally do number 1...  ;)

Took 1!
Comment 25 WebKit Review Bot 2012-02-29 23:50:33 PST
Comment on attachment 129632 [details]
Patch 5

Attachment 129632 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11769257

New failing tests:
fast/dom/shadow/multiple-shadowroot-rendering.html
Comment 26 Brady Eidson 2012-03-01 13:41:51 PST
(In reply to comment #24)
> (From update of attachment 129382 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129382&action=review
> 
> Thank you for the comments.
> 
> >> Source/WebCore/loader/FrameLoader.cpp:3174
> >> +    m_requestedHistoryItem = item;
> > 
> > m_requestedHistoryItem is set in ::loadItem(), but never cleared unless a new loadItem takes place.
> > 
> > Nominally this doesn't seem so bad - Just an extra HistoryItem hanging around that will be destroyed when the FrameLoader is.
> > 
> > But HistoryItems can be surprisingly heavyweight objects, and I think we should clear it out once we don't need it anymore.  Perhaps as the very last part of didFinishLoad?
> 
> didFinishLoad() of what class?

I was going to say FrameLoader...

> I added code to clear the HistoryItem to a place where m_isComplete is set to true.

Not sure this is the right approach, about to look at the patch.
Comment 27 Brady Eidson 2012-03-01 13:44:54 PST
Comment on attachment 129632 [details]
Patch 5

I take back my comment I submitted before reviewing.  This looks good.
Comment 28 Kent Tamura 2012-03-01 16:40:59 PST
Comment on attachment 129632 [details]
Patch 5

Thanks!
Comment 29 WebKit Review Bot 2012-03-01 18:30:15 PST
Comment on attachment 129632 [details]
Patch 5

Clearing flags on attachment: 129632

Committed r109480: <http://trac.webkit.org/changeset/109480>
Comment 30 WebKit Review Bot 2012-03-01 18:30:22 PST
All reviewed patches have been landed.  Closing bug.