Bug 194131 - [WPE][GTK] Load events may occur in unexpected order when JS redirects page before subresource load finishes
Summary: [WPE][GTK] Load events may occur in unexpected order when JS redirects page b...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks: CVE-2019-6251
  Show dependency treegraph
 
Reported: 2019-01-31 17:38 PST by Michael Catanzaro
Modified: 2019-03-12 01:30 PDT (History)
12 users (show)

See Also:


Attachments
Patch (16.88 KB, patch)
2019-02-03 16:41 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (19.80 KB, patch)
2019-02-24 20:58 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (19.72 KB, patch)
2019-02-24 21:01 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (17.49 KB, patch)
2019-03-01 09:29 PST, Michael Catanzaro
rniwa: review-
Details | Formatted Diff | Diff
Patch (9.91 KB, patch)
2019-03-07 03:43 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Fix WPE build (9.92 KB, patch)
2019-03-07 03:55 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (12.68 KB, patch)
2019-03-11 01:05 PDT, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2019-01-31 17:38:22 PST
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.
Comment 1 Michael Catanzaro 2019-01-31 20:48:46 PST
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.)
Comment 2 Michael Catanzaro 2019-02-01 08:17:38 PST
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.
Comment 3 Ryosuke Niwa 2019-02-01 12:09:35 PST
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.
Comment 4 Michael Catanzaro 2019-02-01 12:51:36 PST
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.
Comment 5 Michael Catanzaro 2019-02-01 13:27:41 PST
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.
Comment 6 Michael Catanzaro 2019-02-01 14:43:27 PST
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....
Comment 7 Michael Catanzaro 2019-02-03 16:41:58 PST
Created attachment 361033 [details]
Patch
Comment 8 EWS Watchlist 2019-02-03 16:42:58 PST
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
Comment 9 EWS Watchlist 2019-02-03 16:43:09 PST
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.
Comment 10 Michael Catanzaro 2019-02-03 18:01:41 PST
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."
Comment 11 Michael Catanzaro 2019-02-03 18:02:55 PST
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....
Comment 12 Carlos Garcia Campos 2019-02-04 00:05:59 PST
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.
Comment 13 Michael Catanzaro 2019-02-04 07:16:21 PST
(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.
Comment 14 Carlos Garcia Campos 2019-02-04 07:23:03 PST
(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.
Comment 15 Michael Catanzaro 2019-02-24 19:08:52 PST
(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.
Comment 16 Michael Catanzaro 2019-02-24 20:58:03 PST
Created attachment 362880 [details]
Patch
Comment 17 EWS Watchlist 2019-02-24 21:00:01 PST
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.
Comment 18 Michael Catanzaro 2019-02-24 21:01:02 PST
Created attachment 362881 [details]
Patch
Comment 19 EWS Watchlist 2019-02-24 21:03:24 PST
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.
Comment 20 Carlos Garcia Campos 2019-02-25 02:20:11 PST
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?
Comment 21 Michael Catanzaro 2019-02-25 08:11:30 PST
(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.
Comment 22 Michael Catanzaro 2019-02-28 19:42:36 PST
(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.
Comment 23 Michael Catanzaro 2019-03-01 09:29:00 PST
(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).
Comment 24 Michael Catanzaro 2019-03-01 09:29:15 PST
Created attachment 363333 [details]
Patch
Comment 25 Ryosuke Niwa 2019-03-01 10:56:16 PST
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).
Comment 26 Michael Catanzaro 2019-03-01 12:38:48 PST
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.
Comment 27 Ryosuke Niwa 2019-03-01 12:40:52 PST
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.
Comment 28 Michael Catanzaro 2019-03-01 16:05:03 PST
I mean, we could add a new vfunc to NavigationClient, didStartLoadWithoutFinishingPreviousLoad, something like that.
Comment 29 Carlos Garcia Campos 2019-03-07 01:08:56 PST
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?
Comment 30 Carlos Garcia Campos 2019-03-07 03:43:44 PST
Created attachment 363867 [details]
Patch

What about something like this?
Comment 31 Carlos Garcia Campos 2019-03-07 03:55:18 PST
Created attachment 363868 [details]
Fix WPE build
Comment 32 Michael Catanzaro 2019-03-07 08:55:14 PST
(In reply to Carlos Garcia Campos from comment #30)
> What about something like this?

I like this better than either of mine.
Comment 33 Michael Catanzaro 2019-03-07 08:56:07 PST
(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.
Comment 34 Michael Catanzaro 2019-03-08 07:38:44 PST
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.
Comment 35 Ryosuke Niwa 2019-03-09 18:29:37 PST
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.
Comment 36 Carlos Garcia Campos 2019-03-11 01:05:17 PDT
Created attachment 364239 [details]
Patch
Comment 37 Michael Catanzaro 2019-03-11 09:38:04 PDT
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.
Comment 38 Chris Dumez 2019-03-11 09:39:36 PDT
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.
Comment 39 Michael Catanzaro 2019-03-11 09:42:25 PDT
(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.)
Comment 40 Ryosuke Niwa 2019-03-12 00:13:00 PDT
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.
Comment 41 Carlos Garcia Campos 2019-03-12 01:30:50 PDT
Committed r242788: <https://trac.webkit.org/changeset/242788>