Bug 93680 – Back/Forward list with error page

• Component: History
• Status: UNCONFIRMED

[Basavaraj Padmashali Sidda]

Adding multiple Items in the history tree for a safari error page, i.e when calling loadAlternateHTMLString when back or forward key is pressed.

Steps to reproduce 1. Fetch any URL(eg, google.com) 2. <unreachable URL> which displays safari error page (eg, fdgdfgfdgf.com) 3. 3rd any URL (eg, apple.com) 
when you press back from 3rd page, loadAlternateHTMLString() is called with load type standard which adds one more history item of the error page in the History Tree.
same case with Forward aswell.

Reproducible in safari/WebKit2 nightly build

This issue is similar to <https://bugs.webkit.org/show_bug.cgi?id=84041> which i for the case Reload.

[Basavaraj Padmashali Sidda]

Created attachment 157623 [details]
Patch for back/forward list with error page

Proposed patch for back/forward list with error page

[Basavaraj Padmashali Sidda]

Created attachment 157626 [details]
Updated proposed patch for back/forward list with error page

Updated proposed patch for back/forward list with error page

[Alexey Proskuryakov]

Please mark the patch with review? flag if it's ready for review.

[Basavaraj Padmashali Sidda]

Comment on attachment 157626 [details]
Updated proposed patch for back/forward list with error page

marked review flag to ?

[Darin Adler]

Comment on attachment 157626 [details]
Updated proposed patch for back/forward list with error page

Patches need to come with change logs that explain why the code is changed, and test cases to show the bug and demonstrate the bug has been fixed. review- because this is missing those elements.

[Basavaraj Padmashali Sidda]

Created attachment 176694 [details]
Patch

[Basavaraj Padmashali Sidda]

(In reply to comment #5)
> (From update of attachment 157626 [details])
> Patches need to come with change logs that explain why the code is changed, and test cases to show the bug and demonstrate the bug has been fixed. review- because this is missing those elements.

Thanks for reviewing, i have uploaded the patch with change log. This issue is related to back/forward navigation required from the error page, so i am unable to figure out how to write layout test case.

[Basavaraj Padmashali Sidda]

(In reply to comment #0)
> Adding multiple Items in the history tree for a safari error page, i.e when calling loadAlternateHTMLString when back or forward key is pressed.
> 
> Steps to reproduce 1. Fetch any URL(eg, google.com) 2. <unreachable URL> which displays safari error page (eg, fdgdfgfdgf.com) 3. 3rd any URL (eg, apple.com) 
> when you press back from 3rd page, loadAlternateHTMLString() is called with load type standard which adds one more history item of the error page in the History Tree.
> same case with Forward as well.
> 
> Reproducible in safari/WebKit2 nightly build
> 
> This issue is similar to <https://bugs.webkit.org/show_bug.cgi?id=84041> which i for the case Reload.

PN: This issue can be easily observed without Proxy in webkit2 safari browser,

[WebKit Review Bot]

Comment on attachment 176694 [details]
Patch

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

New failing tests:
fast/dom/DeviceMotion/optional-event-properties.html
http/tests/navigation/redirect302-metaredirect.html
http/tests/navigation/back-to-slow-frame.html
http/tests/navigation/timerredirect-subframeload.html
http/tests/navigation/post-goback2.html
fast/events/pageshow-pagehide.html
fast/forms/button-style-color.html
http/tests/navigation/scrollstate-after-http-equiv-refresh-fragment-identifier-2.html
http/tests/navigation/success200-loadsame.html
http/tests/navigation/reload-subframe-frame.html
http/tests/navigation/go-back-to-error-page.html
http/tests/navigation/changing-frame-hierarchy-in-onload.html
http/tests/history/replacestate-post-to-get.html
fast/forms/range/ValidityState-stepMismatch-range.html
http/tests/navigation/postredirect-frames.html
http/tests/history/back-with-fragment-change.php
compositing/flat-with-transformed-child.html
http/tests/navigation/image-load-in-subframe-unload-handler.html
http/tests/navigation/javascriptlink-subframeload.html
http/tests/navigation/post-frames.html
http/tests/navigation/anchor-subframeload.html
http/tests/cache/subresource-fragment-identifier.html
http/tests/navigation/postredirect-goback2.html
fast/dom/Geolocation/not-enough-arguments.html
fast/dom/navigation-type-navigate.html
http/tests/navigation/postredirect-basic.html
fast/dom/DeviceOrientation/no-synchronous-events.html
fast/events/onunload-clears-onbeforeunload.html
http/tests/navigation/redirect-on-reload-updates-history-item.html
http/tests/navigation/metaredirect-subframeload.html

[Brady Eidson]

Comment on attachment 176694 [details]
Patch

This patch does not include a new test itself.  Requiring tests with behavior changes is a requirement of the project that Darin laid out in his comment.

Additionally it caused existing tests to fail, as the WebKit Review Bot mentioned.

[Basavaraj Padmashali Sidda]

Created attachment 186842 [details]
Patch

[Pravin D]

Comment on attachment 186842 [details]
Patch

Updated the patch to fix the testcase failures. 
Added a testcase for the same

[Pravin D]

(In reply to comment #12)
> (From update of attachment 186842 [details])
> Updated the patch to fix the testcase failures. 
> Added a testcase for the same

Sorry... updated the wrong bug... :( :(

[WebKit Review Bot]

Comment on attachment 186842 [details]
Patch

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

New failing tests:
http/tests/navigation/back-navigation-to-error-page-substitute-data.html

[Brady Eidson]

Comment on attachment 186842 [details]
Patch

Still causes test failures.

[Basavaraj Padmashali Sidda]

Created attachment 188035 [details]
Patch

[Basavaraj Padmashali Sidda]

Comment on attachment 188035 [details]
Patch

Have uploaded a new patch fixing the test case failure on cr-linux.
Chromium behavior towards handling failing URL is different from other ports. Chromium creates a new frame when back/forward navigation is performed. This set the load type to "Standard" by default, even during back/forward navigation.
Have added a chromium port spec expected file for the same.

[Basavaraj Padmashali Sidda]

Please review this patch

[Brady Eidson]

Comment on attachment 188035 [details]
Patch

It doesn't really make sense to me why chromium would be different here.  It seems like they still have the bug.  Is there a good enough explanation for this that we should accept it?

[Basavaraj Padmashali Sidda]

Hi Brady,

This issue is seen in webkit2 code based and not in webkit and chrome, and here is explanation, 

About the issue: 

When load with substitute data, LoadHTMLString() is called with m_loadType is Back/Forward Type for a error page(unreachable URL in our case) in FrameLoader::Load(), the loadType is set to “Reload” if shouldReloadToHandleUnreachableURL() returns true,  Standard Type otherwise. 

shouldReloadToHandleUnreachableURL() return value depends on m_delegateIsHandlingProvisionalLoadError. 

On Webkit(single process) or  when an error page is loaded, m_delegateIsHandlingProvisionalLoadError is set to true. Then a call is made to 

Client’s  dispatchDidFailProvisionalLoad(). This method does a load with substitute data (LoadHTMLString). On  LoadHTMLString , shouldReloadToHandleUnreachableURL () is called. It returns true. As a result the loadType is set to “Reload” and duplication of history item of the error page is not seen on Back/Forward navigation.  Then m_delegateIsHandlingProvisionalLoadError is reset. See the code below:

m_delegateIsHandlingProvisionalLoadError = true;
m_client->dispatchDidFailProvisionalLoad(error); -- sync call of LoadHTMLString() in webkit(single process)
m_delegateIsHandlingProvisionalLoadError = false;

However on webkit2 :
m_delegateIsHandlingProvisionalLoadError = true;
m_client->dispatchDidFailProvisionalLoad(error); -- async call of LoadHTMLString() in webkit(single process)
m_delegateIsHandlingProvisionalLoadError = false;

The call to client’s dispatchDidFailProvisionalLoad() is async. The m_delegateIsHandlingProvisionalLoadError is reset before LoadHTMLString is called from the client. Thus shouldReloadToHandleUnreachableURL() returns false instead of true.  As result a Back/Forward navigation is affected.


Have 2 approaches to fix the issue. 

Approach 1
Implementation of sendSync in client's dispatchDidFailProvisionalLoad(). However this may not be good approach, because this will stall the UI process.
Call flow for the same is WebProcess -> UIProcess -> App -> UIProcess -> Webprocess. Also overwriting loadType to “Reload” on history navigation does not seem to be proper.

Approach2
The proposed patch (https://bug-93680-attachments.webkit.org/attachment.cgi?id=188035). 
In this, we just restore the loadType if it is back/forward type when loading substitute data during Back/Forward Navigation, instead of overwriting loadType to "reload" and loading as reload.

[Basavaraj Padmashali Sidda]

(In reply to comment #20)
> Hi Brady,
> 
> This issue is seen in webkit2 code based and not in webkit and chrome, and here is explanation, 
> 
> About the issue: 
> 
> When load with substitute data, LoadHTMLString() is called with m_loadType is Back/Forward Type for a error page(unreachable URL in our case) in FrameLoader::Load(), the loadType is set to “Reload” if shouldReloadToHandleUnreachableURL() returns true,  Standard Type otherwise. 
> 
> shouldReloadToHandleUnreachableURL() return value depends on m_delegateIsHandlingProvisionalLoadError. 
> 
> On Webkit(single process) or  when an error page is loaded, m_delegateIsHandlingProvisionalLoadError is set to true. Then a call is made to 
> 
> Client’s  dispatchDidFailProvisionalLoad(). This method does a load with substitute data (LoadHTMLString). On  LoadHTMLString , shouldReloadToHandleUnreachableURL () is called. It returns true. As a result the loadType is set to “Reload” and duplication of history item of the error page is not seen on Back/Forward navigation.  Then m_delegateIsHandlingProvisionalLoadError is reset. See the code below:
> 
> m_delegateIsHandlingProvisionalLoadError = true;
> m_client->dispatchDidFailProvisionalLoad(error); -- sync call of LoadHTMLString() in webkit(single process)
> m_delegateIsHandlingProvisionalLoadError = false;
> 
> However on webkit2 :
> m_delegateIsHandlingProvisionalLoadError = true;
> m_client->dispatchDidFailProvisionalLoad(error); -- async call of LoadHTMLString() in webkit(single process)
> m_delegateIsHandlingProvisionalLoadError = false;
> 
> The call to client’s dispatchDidFailProvisionalLoad() is async. The m_delegateIsHandlingProvisionalLoadError is reset before LoadHTMLString is called from the client. Thus shouldReloadToHandleUnreachableURL() returns false instead of true.  As result a Back/Forward navigation is affected.
> 
> 
> Have 2 approaches to fix the issue. 
> 
> Approach 1
> Implementation of sendSync in client's dispatchDidFailProvisionalLoad(). However this may not be good approach, because this will stall the UI process.
> Call flow for the same is WebProcess -> UIProcess -> App -> UIProcess -> Webprocess. Also overwriting loadType to “Reload” on history navigation does not seem to be proper.
> 
> Approach2
> The proposed patch (https://bug-93680-attachments.webkit.org/attachment.cgi?id=188035). 
> In this, we just restore the loadType if it is back/forward type when loading substitute data during Back/Forward Navigation, instead of overwriting loadType to "reload" and loading as reload.

working on approach 2: Reverting existing overwriting of loadType to reload, and on top of it adding proposed patch so that added test case to pass in chrome also and make sure that all existing old test cases to pass ?

[Brady Eidson]

(In reply to comment #20)
> Hi Brady,
> 
> This issue is seen in webkit2 code based and not in webkit and chrome, and here is explanation, 

I was confused by the chromium specific test results.  They clearly still behave differently here.

But it's not important I guess.

[Basavaraj Padmashali Sidda]

Checked with the Aproach2, chrome test results still behaves same,
guess this result is due to some chrome specific change.

[Basavaraj Padmashali Sidda]

In case of Chromium, BackNavigation fails for unreachable url, i.e from 3rd URL to 2nd URL (didFailProvisionalLoad()), 
============== Back Forward List ==============
        http://127.0.0.1:8000/navigation/back-navigation-to-error-page-substitute-data.html  **nav target**
        http://127.0.0.1:2321/SomeUnreachableURL  **nav target**
curr->  http://127.0.0.1:8000/navigation/resources/page-to-go-back-from.html  **nav target**
===============================================
According to test case from patch (https://bug-93680-attachments.webkit.org/attachment.cgi?id=188035), when you call loadHTMLString() after Back Navigation, it overwrites the 3rd URL since m_loadType will be of "Back/Forward" type.
So test result will look different for chromium as shown below.
============== Back Forward List ==============
        http://127.0.0.1:8000/navigation/back-navigation-to-error-page-substitute-data.html  **nav target**
        http://127.0.0.1:2321/SomeUnreachableURL  **nav target**
curr->  http://127.0.0.1:2321/SomeUnreachableURL  **nav target**
===============================================

[Brady Eidson]

I know all of that, I don't know why we're still talking about this - I r+'ed the patch days ago.

[Adam Barth]

@fishd or @creis: Do you know why Chromium has a different result on this test?

[Charles Reis]

(In reply to comment #26)
> @fishd or @creis: Do you know why Chromium has a different result on this test?

I'm not familiar with this code, but I pulled up the repro steps in a debugger.  In Chrome, we appear to hit the shouldReloadToHandleUnreachableURL(newDocumentLoader) check a few lines below this patch's change to FrameLoader::load, causing type to be set to FrameLoadTypeReload.  Maybe that differs with Safari?

I see a scary FIXME around that logic, though, so perhaps someone should revisit it.

[Basavaraj Padmashali Sidda]

(In reply to comment #25)
> I know all of that, I don't know why we're still talking about this - I r+'ed the patch days ago.

> 
Can you please give me a cq+ for the same...

[Basavaraj Padmashali Sidda]

(In reply to comment #27)
> (In reply to comment #26)
> > @fishd or @creis: Do you know why Chromium has a different result on this test?
> 
> I'm not familiar with this code, but I pulled up the repro steps in a debugger.  In Chrome, we appear to hit the shouldReloadToHandleUnreachableURL(newDocumentLoader) check a few lines below this patch's change to FrameLoader::load, causing type to be set to FrameLoadTypeReload.  Maybe that differs with Safari?
> 
This does not seem to be a causing the issue. Even on making  shouldReloadToHandleUnreachableURL() return always false(currently it return true in case of chromium) the test results did not change for chromium port.

> I see a scary FIXME around that logic, though, so perhaps someone should revisit it.
> 
Maybe the different results are due implementation difference in chromium DRT as compared to other ports (didFailProvisionalLoad() is not implemented in WebViewHost).