Bug 206217 – Substitute data lost after reload

• Component: Page Loading
• Status: NEW

[Alexander Kurtakov]

How to reproduce:
1.Run the attached snippet.
2. Observe "Hello World" displayed in the window
3. Right click and from the context menu choose reload.
4. Content changes to file browser displaying content of /

Expected result:
Hello World still displayed.

[Alexander Kurtakov]

Created attachment 387630 [details]
Snippet to reproduce

[Alexander Kurtakov]

Created attachment 387631 [details]
Initial browser window

[Alexander Kurtakov]

Created attachment 387632 [details]
Window after reload

[Alexander Kurtakov]

Happens on Fedora 31 with webkit2gtk3-2.26.2-1.fc31.x86_64 but I got reports that it happens on Fedora 30 too.
Initially reported as https://bugs.eclipse.org/bugs/show_bug.cgi?id=558846

[Carlos Garcia Campos]

This is not port specific.

[Carlos Garcia Campos]

Created attachment 388675 [details]
Patch

[Carlos Garcia Campos]

Created attachment 388676 [details]
Patch

[Michael Catanzaro]

Comment on attachment 388676 [details]
Patch

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

Nice tests! (As usual.)

I notice the red EWS are all failing on preexisting/unrelated contentfiltering layout test failures. Looks like the Mac test results are just in bad state ATM.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestLoaderClient.cpp:239
> +    webkit_web_view_set_custom_charset(test->m_webView, "utf8");

I guess you're testing the reload that occurs when you change the charset? That's far from obvious, so it could use a comment.

[Alex Christensen]

(In reply to Michael Catanzaro from comment #8)
> I notice the red EWS are all failing on preexisting/unrelated
> contentfiltering layout test failures. Looks like the Mac test results are
> just in bad state ATM.
This is not true.  Those are regressions from this patch.  Content filtering uses document loaders and substitute data, and calls reload in PolicyChecker::checkNavigationPolicy

This patch is an API-breaking change which changes behavior that has existed for many, many years.  I don't think we want to do it.

If you load a string that has <script>window.location.reload(true)</script> do we want to change that, too?

[Michael Catanzaro]

Comment on attachment 388676 [details]
Patch

Sorry, I checked the waterfall and noticed the Mac bots were in bad state, but didn't check to see the exact test failures.

[Michael Catanzaro]

(In reply to Alex Christensen from comment #9)
> This patch is an API-breaking change which changes behavior that has existed
> for many, many years.  I don't think we want to do it.

The current behavior is clearly wrong, though. If we reload the page we should reload the current data.

> If you load a string that has <script>window.location.reload(true)</script>
> do we want to change that, too?

It would be a silly thing to do, but why treat it any differently from loading a normal webpage that does that?

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #11)
> The current behavior is clearly wrong, though. If we reload the page we
> should reload the current data.

Actually maybe you're right. If we reload the substitute data, then, for example, we'd wind up reloading an error page instead of attempting to actually reload the page. I bet that would break both Safari and Epiphany.

[Michael Catanzaro]

Comment on attachment 388676 [details]
Patch

After thinking more about this, I'm pretty sure Alex is right. Trying to change this could break a lot of things. Carlos, do you agree?

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #12)
> (In reply to Michael Catanzaro from comment #11)
> > The current behavior is clearly wrong, though. If we reload the page we
> > should reload the current data.
> 
> Actually maybe you're right. If we reload the substitute data, then, for
> example, we'd wind up reloading an error page instead of attempting to
> actually reload the page. I bet that would break both Safari and Epiphany.

Right, in case of having alternate URL we should always load the alternate URL.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #13)
> Comment on attachment 388676 [details]
> Patch
> 
> After thinking more about this, I'm pretty sure Alex is right. Trying to
> change this could break a lot of things. Carlos, do you agree?

I don't mind, TBH, I was just trying to fix this bug. Current behavior doesn't look right for sure, but maybe we can just ignore reloads instead of loading the base URL if we don't want to reload the substituted data.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #15)
> I don't mind, TBH, I was just trying to fix this bug. Current behavior
> doesn't look right for sure,

It doesn't look right when looking at this example, but it's easy to see why we can't change it.

> but maybe we can just ignore reloads instead of
> loading the base URL if we don't want to reload the substituted data.

Say the user hits bug #203620, or a random network failure, or other spurious error, and winds up viewing a network error page. Do you want refresh to be ignored, or to reload the network error page? Of course not: you want it to try reloading the original page. Displaying error pages is even the documented use-case of webkit_web_view_load_alternate_html(). What we need here is better documentation to clarify that the alternate data will not be used when reloaded and that the application is responsible for handling that if required. Or, if this is really important, we could add new API to let the API user choose, but I doubt it's that important.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #16)
> (In reply to Carlos Garcia Campos from comment #15)
> > I don't mind, TBH, I was just trying to fix this bug. Current behavior
> > doesn't look right for sure,
> 
> It doesn't look right when looking at this example, but it's easy to see why
> we can't change it.
> 
> > but maybe we can just ignore reloads instead of
> > loading the base URL if we don't want to reload the substituted data.
> 
> Say the user hits bug #203620, or a random network failure, or other
> spurious error, and winds up viewing a network error page. Do you want
> refresh to be ignored, or to reload the network error page? Of course not:
> you want it to try reloading the original page. Displaying error pages is
> even the documented use-case of webkit_web_view_load_alternate_html(). What
> we need here is better documentation to clarify that the alternate data will
> not be used when reloaded and that the application is responsible for
> handling that if required. Or, if this is really important, we could add new
> API to let the API user choose, but I doubt it's that important.

I said in the previous comment that in case of having an alternate URL we should always load the alternate URL. The question is what to do in all other cases.

[Michael Catanzaro]

OK, I don't know. :)