Bug 206217

Created attachment 387630 [details] Snippet to reproduce

Created attachment 387631 [details] Initial browser window

Created attachment 387632 [details] Window after reload

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

This is not port specific.

Created attachment 388675 [details] Patch

Created attachment 388676 [details] Patch

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.

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

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.

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

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

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?

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

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

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

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

OK, I don't know. :)