Bug 203620 – [SOUP] HSTS Support causes page loading to fail with "Operation was cancelled"

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Patrick Griffis]

Steps to reproduce:

Load HSTS using site like `http://google.com`, upon redirection it will often but not always fail.

It also hits the soupRequest assertion in `NetworkDataTaskSoup::sendRequestCallback()` in these cases.



Modifying libsoup to never return a policy made the issue go away.

This was originally reported against GLib: https://gitlab.gnome.org/GNOME/glib/issues/1891

[Claudio Saavedra]

I can hit this in ephy but not in minibrowser.

[Michael Catanzaro]

Just hit this bug twice in a row when loading http://webkitgtk.org. On the third attempt, the load worked. I can no longer reproduce.

[Claudio Saavedra]

MiniBrowser or Epiphany?

[Michael Catanzaro]

Epiphany

[Claudio Saavedra]

I'm wondering if it's possible that the issue is in Epiphany.. because I couldn't reproduce this at all in MiniBrowser.

[Michael Catanzaro]

(In reply to Patrick Griffis from comment #0)
> Steps to reproduce:
> 
> Load HSTS using site like `http://google.com`, upon redirection it will
> often but not always fail.
> 
> It also hits the soupRequest assertion in
> `NetworkDataTaskSoup::sendRequestCallback()` in these cases.

Well you're hitting an assertion in WebKit network process. That's definitely not Epiphany's fault.

[Patrick Griffis]

Also I used Cog+WPE for reproducing this, Epiphany isn't relevant.

[Michael Catanzaro]

Ping, this is important....

[Carlos Garcia Campos]

I've never seen this error and I can't reproduce it.

[Michael Catanzaro]

It has never been reproducible. But fortunately, Patrick has already debugged it fairly well:

(In reply to Patrick Griffis from comment #0)
> It also hits the soupRequest assertion in
> `NetworkDataTaskSoup::sendRequestCallback()` in these cases.
> 
> 
> 
> Modifying libsoup to never return a policy made the issue go away.

[Patrick Griffis]

With wpewebkit-2.27.1 I can reproduce extremely consistently.

Changing no deps but running against r253428 (git ac429352178b2ff4460e775cfd4b1fb79621d4ad) I cannot reproduce at all.

So I'm inclined to believe it was resolved unless Michael can reproduce it against trunk.

[Michael Catanzaro]

I've never been able to reproduce it consistently. So if you're confident it's solved, then that's great. It's very weird an issue like this would just resolve itself, though.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #12)
> I've never been able to reproduce it consistently.

I went back and built WebKitGTK 2.27.1, deleted my HSTS database, and loaded http://google.com. I was successfully redirected to https. Then I closed Epiphany, deleted my HSTS database again, and repeated the process several times. I was never able to reproduce. So we'll need to rely on your observations here.

Of course, I'll reopen if I see it again.

[Carlos Garcia Campos]

I can reproduce this now with WPE MiniBrowser and cog. The problem is that we are assuming that request cancellation happens synchronously, but it can happen that the async ready callback for the previous request is called after the new one has started.

[Carlos Garcia Campos]

Created attachment 386681 [details]
Patch

[Michael Catanzaro]

Comment on attachment 386681 [details]
Patch

Aha, nice find!

One problem though: the user data parameter NetworkDataTaskSoup* task is passed to this callback using leakRef(), but now you've added this new early return before the ref is adopted, causing the task to leak. So this needs to go down below that at least. How about like this:

RefPtr<NetworkDataTaskSoup> protectedThis = adoptRef(task);
if (soupRequest != task->m_soupRequest.get()) {
    // ...
}
if (task->state() == State::Canceling || task->state() == State::Completed || !task->m_client) {
    // ...
}

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #16)
> RefPtr<NetworkDataTaskSoup> protectedThis = adoptRef(task);

Well, also protectedThis is the wrong name because it's not a protector, it's just required to adopt the ref.

[Michael Catanzaro]

Comment on attachment 386681 [details]
Patch

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

> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:310
>      RefPtr<NetworkDataTaskSoup> protectedThis = adoptRef(task);

Maybe we should do a global search to see if we have more places where the words "protected" or "protector" and "adopt" appear on the same line. Seems that will almost always indicate either a misnamed variable or, potentially, a bug. If it really *was* a protector (no adopt), then your change would have been fine as-is. I think you got tricked by the incorrect name.

[Carlos Garcia Campos]

You are right, and it was indeed the name protectedThis what confused me.

[Carlos Garcia Campos]

Committed r254119: <https://trac.webkit.org/changeset/254119>