REGRESSION(r106388): Form state is restored to a wrong document

• Component: History
• Status: RESOLVED FIXED

[Kent Tamura]

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.

[Kent Tamura]

Created attachment 128149 [details]
Patch

[Dimitri Glazkov (Google)]

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?

[Alexey Proskuryakov]

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?

[Kent Tamura]

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.

[Kent Tamura]

Created attachment 128354 [details]
Patch 2

string -> KURL

[Kent Tamura]

I need comments from loader/history experts.
Adam, Antti, Brady, Mihai?

[Adam Barth]

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.

[Brady Eidson]

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

[Adam Barth]

Don't we already have a sequence number for documents?

[Kent Tamura]

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

[Brady Eidson]

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

[Kent Tamura]

Created attachment 128961 [details]
Patch 3

HistoryItem::isCurrentDocument

[Kent Tamura]

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

[Brady Eidson]

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.

[Brady Eidson]

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)

[Kent Tamura]

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

[Brady Eidson]

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

[Kent Tamura]

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

[Brady Eidson]

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

[Kent Tamura]

Created attachment 129382 [details]
Patch 4

[Kent Tamura]

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

[Brady Eidson]

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...  ;)

[Kent Tamura]

Created attachment 129632 [details]
Patch 5

[Kent Tamura]

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!

[WebKit Review Bot]

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

[Brady Eidson]

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

[Brady Eidson]

Comment on attachment 129632 [details]
Patch 5

I take back my comment I submitted before reviewing.  This looks good.

[Kent Tamura]

Comment on attachment 129632 [details]
Patch 5

Thanks!

[WebKit Review Bot]

Comment on attachment 129632 [details]
Patch 5

Clearing flags on attachment: 129632

Committed r109480: <http://trac.webkit.org/changeset/109480>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.