Bug 194131 – [WPE][GTK] Load events may occur in unexpected order when JS redirects page before subresource load finishes

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

Save the following as test.html and load it in MiniBrowser:

<img src="https://www.example.com:8080/example.png"/>
<script>
function run()
{
    location = "https://www.example.com:8080";
}

setInterval(run(),100000);
</script>

Open that with WEBKIT_DEBUG="loading" and observe the load proceeds as follows: didStartProvisionalLoadForFrame -> didCommitLoadForFrame -> didStartProvisionalLoadForFrame.

But I think the expected behavior would be  didStartProvisionalLoadForFrame -> didCommitLoadForFrame -> didFinishLoadForFrame -> didStartProvisionalLoadForFrame. Right?

Applications (at least applications using the GTK/WPE APIs) expect load events to occur in the well-defined order start -> reload (optional) -> committed -> finished. When start occurs before finished, very strange bugs occur. It would be nice to fix this in WebCore rather than papering over it at the GTK/WPE API layer. (We used to paper over weird load event quirks there, but it resulted in many bugs, and I think this is the first trouble we've found since we stopped doing so.)

Note: this is related to Epiphany CVE-2019-6251.

[Michael Catanzaro]

After a few hours staring at FrameLoader.cpp, I think the current behavior of WebCore is OK -- because the load really never finishes -- and we could choose to just run the finished event at the WPE/GTK API level. Alternatively, it looks like FrameLoader::setState would be a good place to run m_client.dispatchDidFinishLoad to fake the finish, but I'd worry about unintended side effects. CCing people who've touched loader and NavigationClient for opinions on intended behavior.

P.S. In this example, didFinishDocumentLoadForFrame is executed, but the subresource never finishes because there's no HTTP server running on www.example.com:8080, so didFinishLoadForFrame never runs. (We have to use a valid host to ensure it resolves, or the load will fail.)

[Michael Catanzaro]

We could also add a new callback to FrameLoaderClient to indicate a load has been cancelled without being finished, but it would just always be called immediately before load started, so it's not needed. The higher layers have enough information to add it.

[Ryosuke Niwa]

I'm not sure there is anything wrong with the current behavior in WebCore since the load never finished before new one had started.

I guess there is nothing with adding a new delegate callback to indicate that the previous load had been canceled / stopped before new load started.

[Michael Catanzaro]

I'm leaning towards just modifying the WPE/GTK API layer for now, and leave WebCore unchanged unless Carlos Garcia suggests otherwise.

And I've discovered that WPE/GTK API does not actually guarantee LOAD_COMMITTED always occurs if there is an error. Only LOAD_FINISHED is guaranteed. So it's actually legit for us to skip LOAD_COMMITTED (although it's not being skipped in my test case there). But I'll document this, because otherwise code might expect that to only be skipped on error.

[Michael Catanzaro]

I'm planning to add this documentation:

     * If a new load is started when the previous load has not yet
     * finished, or if the load fails, then %WEBKIT_LOAD_COMMITTED may
     * or may not be emitted. In contrast, %WEBKIT_LOAD_FINISHED is
     * guaranteed to be emitted before the next %WEBKIT_LOAD_STARTED,
     * even if the load did not actually complete.

Carlos, does it look OK? It has the disadvantage that apps no longer have any way to detect the load has been cancelled by looking for %WEBKIT_LOAD_FINISHED to be skipped. We could alternatively document that %WEBKIT_LOAD_FINISHED is simply not guaranteed to occur and expect that apps be prepared for it.

[Michael Catanzaro]

Yet another alternative would be to treat it as a cancellation and emit load-failed *and* LOAD_FINISHED. We have a lot of options here....

[Michael Catanzaro]

Created attachment 361033 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[EWS Watchlist]

Attachment 361033 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/API/glib/WebKitNavigationClient.cpp:49:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Comment on attachment 361033 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitNavigationClient.cpp:71
> +            // ensure the Finished event occurs. (But Committed may skipped.)

Should read: "But Committed may be skipped."

[Michael Catanzaro]

Comment on attachment 361033 [details]
Patch

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

> Source/WebKit/ChangeLog:11
> +        If a new page load starts before the previous one completes, run LOAD_FINISHED before
> +        LOAD_STARTED to avoid confusing applications that expect LOAD_FINISHED to occur. We could
> +        also perhaps run load-failed as well, with a cancellation error, but this patch doesn't
> +        implement that.

BTW I think we probably should run load-failed, otherwise  it looks like a successful load. That's a bit harder, though, and probably WebCore should emit the error....

[Carlos Garcia Campos]

I'm not sure I understand. When a new load starts, previous ongoing load should be cancelled, no? What I would expect is that load-failed is called with cancelled error, which is generally handled as a special case by applications (not showing any error message). I don't understand the committed behavior either, do you mean that committed is not called even after the ongoing load was already committed? or is it because the load is cancelled before being committed? The former sounds like a bug to me, the latter is what I would expect.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #12)
> I'm not sure I understand. When a new load starts, previous ongoing load
> should be cancelled, no? What I would expect is that load-failed is called
> with cancelled error, which is generally handled as a special case by
> applications (not showing any error message).

Yeah, I think I want to do that too. The current behavior is that it transitions from COMMITTED -> STARTED, skipping FINISHED and not emitting load-failed at all. My patch adds in only the FINISHED. I think I agree that it would be best to do load-failed as well. The implementation problem is we then need to either (a) remember in the navigation client the URI we are loading, so we can fake it when emitting load-failed, which isn't great, or else (b) change WebCore to emit the cancellation, which is probably too hard for me.

> I don't understand the
> committed behavior either, do you mean that committed is not called even
> after the ongoing load was already committed?

No.

> or is it because the load is
> cancelled before being committed? The former sounds like a bug to me, the
> latter is what I would expect.

Right, I agree we should expect it, so my patch just clarifies that in the documentation.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #13)
> (In reply to Carlos Garcia Campos from comment #12)
> > I'm not sure I understand. When a new load starts, previous ongoing load
> > should be cancelled, no? What I would expect is that load-failed is called
> > with cancelled error, which is generally handled as a special case by
> > applications (not showing any error message).
> 
> Yeah, I think I want to do that too. The current behavior is that it
> transitions from COMMITTED -> STARTED, skipping FINISHED and not emitting
> load-failed at all. My patch adds in only the FINISHED. I think I agree that
> it would be best to do load-failed as well. The implementation problem is we
> then need to either (a) remember in the navigation client the URI we are
> loading, so we can fake it when emitting load-failed, which isn't great, or
> else (b) change WebCore to emit the cancellation, which is probably too hard
> for me.

I don't see why it would be too hard for you.

> > I don't understand the
> > committed behavior either, do you mean that committed is not called even
> > after the ongoing load was already committed?
> 
> No.
> 
> > or is it because the load is
> > cancelled before being committed? The former sounds like a bug to me, the
> > latter is what I would expect.
> 
> Right, I agree we should expect it, so my patch just clarifies that in the
> documentation.

I think it makes it more confusing, that's why I thought you meant committed was not emitted even when the load was committed.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #14)
> > Yeah, I think I want to do that too. The current behavior is that it
> > transitions from COMMITTED -> STARTED, skipping FINISHED and not emitting
> > load-failed at all. My patch adds in only the FINISHED. I think I agree that
> > it would be best to do load-failed as well. The implementation problem is we
> > then need to either (a) remember in the navigation client the URI we are
> > loading, so we can fake it when emitting load-failed, which isn't great, or
> > else (b) change WebCore to emit the cancellation, which is probably too hard
> > for me.
> 
> I don't see why it would be too hard for you.

I spent several hours investigating this. It's hard. As far as I can tell, WebCore isn't the right place after all, since the FrameLoader for the previous page load is different than the FrameLoader for the new load, so I guess it should be solved at a higher level than FrameLoader. The next-higher level is WebFrameLoaderClient, which is surely the wrong place, and then WebPageProxy. But WebPageProxy's PageLoadState member is carefully designed to make it impossible to actually check the current page load state, which is frustrating. And then the next-higher level is WPE/GTK NavigationClient, so then we're back to working around this at the API level.

[Michael Catanzaro]

Created attachment 362880 [details]
Patch

[EWS Watchlist]

Attachment 362880 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/API/glib/WebKitNavigationClient.cpp:57:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Created attachment 362881 [details]
Patch

[EWS Watchlist]

Attachment 362881 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/API/glib/WebKitNavigationClient.cpp:57:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 362881 [details]
Patch

Do we really need to add another page load state tracker? Why can't we do this in WebPageProxy with PageLoadState?

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #20)
> Comment on attachment 362881 [details]
> Patch
> 
> Do we really need to add another page load state tracker? Why can't we do
> this in WebPageProxy with PageLoadState?

We could do it for this bug, at the cost of having an API-level state machine, which is nice because it all but guarantees it won't be changed except by someone familiar with WPE/GTK API. But anyway, yes, I think we can do it in PageLoadState. For this bug, all we'd need to do is have WebPageProxy emit extra loadFailed events on the navigation/loader client. Should be easy, and possible to get owner approval for this.

But then we need to consider bug #194208 as well, which is the next part. And that gets trickier, since it requires us to maintain a fake URL, which would then have to be done in WebPageProxy instead of WebKitWebView, as I had been planning. That might be hard to get owner approval for. Maybe you'd better look at my patch in that bug now, too. If we want to do this in WebPageProxy, it will probably need to become one bug report with the two handled together.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #21)
> But then we need to consider bug #194208 as well, which is the next part.
> And that gets trickier, since it requires us to maintain a fake URL, which
> would then have to be done in WebPageProxy instead of WebKitWebView, as I
> had been planning.

That's not true, it can still be done in WebKitWebView. Problem is we need somewhere to trigger the call to webkitWebViewResetToLastCommittedURI. If we do the reset only when a new load starts before the previous one finished, then that would require a new vfunc in APINavigationClient, which would be nice to avoid. But I think if we do the reset always for any cancelled load, we would not need a new vfunc and things should be fine. Let me try that.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #20)
> Do we really need to add another page load state tracker? Why can't we do
> this in WebPageProxy with PageLoadState?

Here's a new version. Needs owner approval (CC: Alex and Youenn).

[Michael Catanzaro]

Created attachment 363333 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 363333 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:3895
> +            m_navigationClient->didFailProvisionalNavigationWithError(*this, *frame, previousNavigation, { ResourceError::Type::Cancellation }, process->transformHandlesToObjects(userData.object()).get());
> +        else if (previousState == PageLoadState::State::Committed)
> +            m_navigationClient->didFailNavigationWithError(*this, *frame, previousNavigation, { ResourceError::Type::Cancellation }, m_process->transformHandlesToObjects(userData.object()).get());

I don't think invoking clients in this case is backwards compatible change.
Please make this change in such a way that it won't affect Apple maintained ports (macOS / iOS).

[Michael Catanzaro]

OK, I've obsoleted the new patch (attachment #363333 [details]) and requested r? again for the previous platform-specific version of the patch (attachment #362881 [details]).

(In reply to Carlos Garcia Campos from comment #20)
> Do we really need to add another page load state tracker? Why can't we do
> this in WebPageProxy with PageLoadState?

That actually was almost nice, but doesn't do us any good if Apple doesn't want it. So back to the original patch. Please remember this is blocking a security bug that was reported several months ago.

[Ryosuke Niwa]

Note that updating PageLoadState would be okay. It's just that we need some mechanism to preserve the old behavior in our ports. But GTK+ port specific solution would be okay too.

[Michael Catanzaro]

I mean, we could add a new vfunc to NavigationClient, didStartLoadWithoutFinishingPreviousLoad, something like that.

[Carlos Garcia Campos]

Comment on attachment 363333 [details]
Patch

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

I think the behavior has changed in WebCore at some point. We need to update the existing unit tests, because we have different expectations see:

https://trac.webkit.org/browser/webkit/trunk/Tools/TestWebKitAPI/glib/WebKitGLib/LoadTrackingTest.cpp#L54
https://trac.webkit.org/browser/webkit/trunk/Tools/TestWebKitAPI/glib/WebKitGLib/LoadTrackingTest.cpp#L74

We expect the load to be cancelled when a new load starts. We need to update that in the tests too.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:3895
>> +            m_navigationClient->didFailNavigationWithError(*this, *frame, previousNavigation, { ResourceError::Type::Cancellation }, m_process->transformHandlesToObjects(userData.object()).get());
> 
> I don't think invoking clients in this case is backwards compatible change.
> Please make this change in such a way that it won't affect Apple maintained ports (macOS / iOS).

This is all happening in the UI process, but the previous load will not be actually stopped, shouldn't we ensure FrameLoader::stopForUserCancel() is called in the web process?

[Carlos Garcia Campos]

Created attachment 363867 [details]
Patch

What about something like this?

[Carlos Garcia Campos]

Created attachment 363868 [details]
Fix WPE build

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #30)
> What about something like this?

I like this better than either of mine.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #29)
> This is all happening in the UI process, but the previous load will not be
> actually stopped, shouldn't we ensure FrameLoader::stopForUserCancel() is
> called in the web process?

It's not necessarily a user cancel, though. E.g. in the case we are testing, the load is cancelled by JavaScript, not by API request.

[Michael Catanzaro]

Comment on attachment 363868 [details]
Fix WPE build

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

> Source/WebKit/UIProcess/PageLoadState.h:124
> +    State state() const { return m_committedState.state; }

Ah, I think this is probably not exposed by design... that's why my patches are so much more complex. Everything is a lot easier if we expose PageLoadState::state.

It's not the first time I've wanted to be able to get at the State of a PageLoadState, so if Apple is OK with this, then great.

[Ryosuke Niwa]

Comment on attachment 363868 [details]
Fix WPE build

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

>> Source/WebKit/UIProcess/PageLoadState.h:124
>> +    State state() const { return m_committedState.state; }
> 
> Ah, I think this is probably not exposed by design... that's why my patches are so much more complex. Everything is a lot easier if we expose PageLoadState::state.
> 
> It's not the first time I've wanted to be able to get at the State of a PageLoadState, so if Apple is OK with this, then great.

Yeah, let's add isFinished() instead. We don't want to repeat the mistake of WebCore where every class depends on each other's states.

[Carlos Garcia Campos]

Created attachment 364239 [details]
Patch

[Michael Catanzaro]

Comment on attachment 364239 [details]
Patch

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

> Source/WebKit/ChangeLog:17
> +        (API::ViewClient::willStartLoad): Add willStartLoad() to API::ViowClient

ViewClient

> Source/WebKit/UIProcess/PageLoadState.h:126
>      bool isLoading() const;
> +    bool isProvisional() const { return m_committedState.state == State::Provisional; }
> +    bool isCommitted() const { return m_committedState.state == State::Committed; }
> +    bool isFinished() const { return m_committedState.state == State::Finished; }

I think we should do this, because easy problems are unnecessarily hard if we don't expose the load state of PageLoadState. (You can see my previous two attempts at this fix.) It needs owner approval, though. This is the only part of the patch that requires an owner. The rest is r=me.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestLoaderClient.cpp:247
> +    const char* uri = webkit_web_view_get_uri(webView);
> +    if (g_str_has_suffix(uri, "/normal"))
> +        test->m_activeURI = uri;

Smart.

[Chris Dumez]

Comment on attachment 364239 [details]
Patch

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

>> Source/WebKit/UIProcess/PageLoadState.h:126
>> +    bool isFinished() const { return m_committedState.state == State::Finished; }
> 
> I think we should do this, because easy problems are unnecessarily hard if we don't expose the load state of PageLoadState. (You can see my previous two attempts at this fix.) It needs owner approval, though. This is the only part of the patch that requires an owner. The rest is r=me.

LGTM.

[Michael Catanzaro]

(In reply to Ryosuke Niwa from comment #35)
> Yeah, let's add isFinished() instead. We don't want to repeat the mistake of
> WebCore where every class depends on each other's states.

Oh, I missed this comment. We need more than just isFinished(), since the load-failed signal is emitted differently based on whether the load is provisional or committed. (Alternative is to build our own API-layer state machine, which Carlos is trying to avoid here.)

[Ryosuke Niwa]

Yet another approach here is to make this behavior configurable in WebCore, or add new delegate callbacks in WebCore. For example, it's possible that the proposed behavior in GTK+ is also compatible with existing iOS / macOS apps but we need a way to toggle it so that we can disable it if it's not.

[Carlos Garcia Campos]

Committed r242788: <https://trac.webkit.org/changeset/242788>