Bug 139342 - Pending API Request URL is wrong after reloading
Summary: Pending API Request URL is wrong after reloading
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 136916
Blocks:
  Show dependency treegraph
 
Reported: 2014-12-06 09:58 PST by Carlos Garcia Campos
Modified: 2016-04-18 03:36 PDT (History)
8 users (show)

See Also:


Attachments
Patch (8.11 KB, patch)
2014-12-06 10:05 PST, Carlos Garcia Campos
darin: review+
Details | Formatted Diff | Diff
Patch for landing (8.31 KB, patch)
2016-04-18 01:58 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased patch (8.31 KB, patch)
2016-04-18 03:03 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2014-12-06 09:58:41 PST
It happens when reloading a web view loaded with anything but a URL, because the bf list is not updated for those cases and WebPageProxy::reload() is setting the current bf list item URL as Pending API Request URL. This also causes that progress is reported wrongly, because WebPageProxy::decidePolicyForNavigationAction() resets the Pending API Request URL when it's different than the requested URL. The page load transaction causes the progress to be changed, reporting 1.0 (the previous one), but later something < 1.0 is reported again by the progress tracker.
Comment 1 Carlos Garcia Campos 2014-12-06 10:05:13 PST
Created attachment 242716 [details]
Patch

I've reused the new test I added for bug #136916 to test this.
Comment 2 Michael Catanzaro 2016-01-02 10:05:39 PST
Owners?
Comment 3 Darin Adler 2016-01-15 10:20:44 PST
Comment on attachment 242716 [details]
Patch

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

I’m saying review+ but I am not 100% sure this is right.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:902
> +    String url = m_pageLoadState.activeURL();
> +    if (url.isEmpty() && m_backForwardList->currentItem())
> +        url = m_backForwardList->currentItem()->url();
> +
> +    if (!url.isEmpty()) {

Seems a little peculiar here to use isEmpty rather than isNull.

I’m generally concerned that this code is trying to figure out what URL the other process will reload; we are not sending the URL over, so this is trying to predict what the other process will do? If so, then it seems that to be correct this has to match the logic in FrameLoader::reload and other loader logic inside WebCore, and it seems to me that adding this one thing it’s still far from matching it! It’s nice to fix this one particular case, but what about:

- The case where there is an unreachableURL.
- If a window is created by javascript, its main frame can have an empty but non-nil URL. We need to test that case.

I’d like to hear Anders’s opinion.
Comment 4 Carlos Garcia Campos 2016-02-02 04:02:25 PST
(In reply to comment #3)
> Comment on attachment 242716 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242716&action=review
> 
> I’m saying review+ but I am not 100% sure this is right.
> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:902
> > +    String url = m_pageLoadState.activeURL();
> > +    if (url.isEmpty() && m_backForwardList->currentItem())
> > +        url = m_backForwardList->currentItem()->url();
> > +
> > +    if (!url.isEmpty()) {
> 
> Seems a little peculiar here to use isEmpty rather than isNull.
> 
> I’m generally concerned that this code is trying to figure out what URL the
> other process will reload; we are not sending the URL over, so this is
> trying to predict what the other process will do? If so, then it seems that
> to be correct this has to match the logic in FrameLoader::reload and other
> loader logic inside WebCore, and it seems to me that adding this one thing
> it’s still far from matching it!

Note that this only affects the pending API request URL. This is to ensure that getting the active URL right after load or reload returns a valid URL, in case of load methods the one passed to the load. In case of reload that URL should always be the current active URL of the web view, since that's what we want to reload. If the web process ends up loading a different URL it doesn't really matter, because as soon as provisional load starts, the provisional url is used  instead of the pending api request one.

> It’s nice to fix this one particular case,
> but what about:
> 
> - The case where there is an unreachableURL.

This case is covered by this patch, because PageLoadState::activeURL() returns the unreachable URL if there's one.

> - If a window is created by javascript, its main frame can have an empty but
> non-nil URL. We need to test that case.

Yes, I would need to check what happens in this particular case.

> I’d like to hear Anders’s opinion.
Comment 5 Carlos Garcia Campos 2016-04-18 01:58:11 PDT
Created attachment 276622 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2016-04-18 01:59:13 PDT
Attachment 276622 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKit2/PendingAPIRequestURL.cpp:44:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Carlos Garcia Campos 2016-04-18 03:03:51 PDT
Created attachment 276629 [details]
Rebased patch
Comment 8 WebKit Commit Bot 2016-04-18 03:05:47 PDT
Attachment 276629 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKit2/PendingAPIRequestURL.cpp:47:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Carlos Garcia Campos 2016-04-18 03:36:31 PDT
Committed r199664: <http://trac.webkit.org/changeset/199664>