203620 2019-10-30 11:26:16 -0700 [SOUP] HSTS Support causes page loading to fail with "Operation was cancelled" 2020-01-07 01:58:08 -0800 1 1 1 Unclassified WebKit WebKitGTK WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=204736 P2 Normal --- 1 pgriffis webkit-unassigned bugs-noreply cgarcia csaavedra mcatanzaro oldest_to_newest 1585438 0 pgriffis 2019-10-30 11:26:16 -0700 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 1585842 1 csaavedra 2019-10-31 03:42:33 -0700 I can hit this in ephy but not in minibrowser. 1588623 2 mcatanzaro 2019-11-08 06:55:35 -0800 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. 1589165 3 csaavedra 2019-11-11 04:53:01 -0800 MiniBrowser or Epiphany? 1589168 4 mcatanzaro 2019-11-11 05:24:49 -0800 Epiphany 1589171 5 csaavedra 2019-11-11 05:29:24 -0800 I'm wondering if it's possible that the issue is in Epiphany.. because I couldn't reproduce this at all in MiniBrowser. 1589179 6 mcatanzaro 2019-11-11 06:31:14 -0800 (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. 1589337 7 pgriffis 2019-11-11 14:47:33 -0800 Also I used Cog+WPE for reproducing this, Epiphany isn't relevant. 1596923 8 mcatanzaro 2019-12-09 12:47:42 -0800 Ping, this is important.... 1598809 9 cgarcia 2019-12-16 01:47:07 -0800 I've never seen this error and I can't reproduce it. 1598843 10 mcatanzaro 2019-12-16 05:47:24 -0800 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. 1599267 11 pgriffis 2019-12-16 19:53:44 -0800 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. 1599422 12 mcatanzaro 2019-12-17 08:07:41 -0800 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. 1599423 13 mcatanzaro 2019-12-17 08:11:10 -0800 (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. 1602415 14 cgarcia 2020-01-03 07:14:32 -0800 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. 1602416 15 386681 cgarcia 2020-01-03 07:16:52 -0800 Created attachment 386681 Patch 1602453 16 386681 mcatanzaro 2020-01-03 09:00:26 -0800 Comment on attachment 386681 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) { // ... } 1602456 17 mcatanzaro 2020-01-03 09:02:28 -0800 (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. 1602728 18 386681 mcatanzaro 2020-01-03 20:49:02 -0800 Comment on attachment 386681 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. 1602840 19 cgarcia 2020-01-05 05:20:17 -0800 You are right, and it was indeed the name protectedThis what confused me. 1603495 20 cgarcia 2020-01-07 01:58:08 -0800 Committed r254119: <https://trac.webkit.org/changeset/254119> 386681 2020-01-03 07:16:52 -0800 2020-01-03 09:00:26 -0800 Patch wk2-hsts-cancelled.diff text/plain 2424 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nIGIvU291cmNlL1dlYktpdC9DaGFu Z2VMb2cKaW5kZXggMTg5YTRjOTRjNTkuLjRhZDNmNmE5YWU0IDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViS2l0L0NoYW5nZUxvZworKysgYi9Tb3VyY2UvV2ViS2l0L0NoYW5nZUxvZwpAQCAtMSwzICsx LDE2IEBACisyMDIwLTAxLTAzICBDYXJsb3MgR2FyY2lhIENhbXBvcyAgPGNnYXJjaWFAaWdhbGlh LmNvbT4KKworICAgICAgICBbU09VUF0gSFNUUyBTdXBwb3J0IGNhdXNlcyBwYWdlIGxvYWRpbmcg dG8gZmFpbCB3aXRoICJPcGVyYXRpb24gd2FzIGNhbmNlbGxlZCIKKyAgICAgICAgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIwMzYyMAorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoZSBwcm9ibGVtIGlzIHRoYXQgd2UgYXJl IGFzc3VtaW5nIHRoYXQgcmVxdWVzdCBjYW5jZWxsYXRpb24gaGFwcGVucyBzeW5jaHJvbm91c2x5 LCBidXQgaXQgY2FuIGhhcHBlbiB0aGF0IHRoZQorICAgICAgICBhc3luYyByZWFkeSBjYWxsYmFj ayBmb3IgdGhlIHByZXZpb3VzIHJlcXVlc3QgaXMgY2FsbGVkIGFmdGVyIHRoZSBuZXcgb25lIGhh cyBzdGFydGVkLgorCisgICAgICAgICogTmV0d29ya1Byb2Nlc3Mvc291cC9OZXR3b3JrRGF0YVRh c2tTb3VwLmNwcDoKKyAgICAgICAgKFdlYktpdDo6TmV0d29ya0RhdGFUYXNrU291cDo6c2VuZFJl cXVlc3RDYWxsYmFjayk6IFJldHVybiBlYXJseSBpZiB0aGlzIGlzIGEgcHJldmlvdXMgcmVxdWVz dCBhbHJlYWR5IGNhbmNlbGxlZC4KKwogMjAxOS0xMi0yNyAgQ2FybG9zIEdhcmNpYSBDYW1wb3Mg IDxjZ2FyY2lhQGlnYWxpYS5jb20+CiAKICAgICAgICAgW0dUS11bV1BFXSBBZGQgQVBJIHRvIHNl dCBwdXJwb3NlIGFuZCBoaW50cyBvZiBhY3RpdmUgZWRpdGFibGUgZWxlbWVudCB0byBpbnB1dCBt ZXRob2RzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L05ldHdvcmtQcm9jZXNzL3NvdXAvTmV0 d29ya0RhdGFUYXNrU291cC5jcHAgYi9Tb3VyY2UvV2ViS2l0L05ldHdvcmtQcm9jZXNzL3NvdXAv TmV0d29ya0RhdGFUYXNrU291cC5jcHAKaW5kZXggZmNiMjYwNWNjZDAuLmUyZTNhMDlmMzcyIDEw MDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0L05ldHdvcmtQcm9jZXNzL3NvdXAvTmV0d29ya0RhdGFU YXNrU291cC5jcHAKKysrIGIvU291cmNlL1dlYktpdC9OZXR3b3JrUHJvY2Vzcy9zb3VwL05ldHdv cmtEYXRhVGFza1NvdXAuY3BwCkBAIC0yOTUsMTIgKzI5NSwyMyBAQCB2b2lkIE5ldHdvcmtEYXRh VGFza1NvdXA6OnN0b3BUaW1lb3V0KCkKIAogdm9pZCBOZXR3b3JrRGF0YVRhc2tTb3VwOjpzZW5k UmVxdWVzdENhbGxiYWNrKFNvdXBSZXF1ZXN0KiBzb3VwUmVxdWVzdCwgR0FzeW5jUmVzdWx0KiBy ZXN1bHQsIE5ldHdvcmtEYXRhVGFza1NvdXAqIHRhc2spCiB7CisgICAgaWYgKHNvdXBSZXF1ZXN0 ICE9IHRhc2stPm1fc291cFJlcXVlc3QuZ2V0KCkpIHsKKyAgICAgICAgLy8gVGhpcyBjYW4gaGFw cGVuIHdoZW4gdGhlIHJlcXVlc3QgaXMgY2FuY2VsbGVkIGFuZCBhIG5ldyBvbmUgaXMgc3RhcnRl ZCBiZWZvcmUKKyAgICAgICAgLy8gdGhlIHByZXZpb3VzIGFzeW5jIG9wZXJhdGlvbiBjb21wbGV0 ZWQuIFRoaXMgaXMgY29tbW9uIHdoZW4gZm9yY2luZyBhIHJlZGlyZWN0aW9uCisgICAgICAgIC8v IGR1ZSB0byBIU1RTLiBXZSBjYW4gc2ltcGx5IGlnbm9yZSB0aGlzIG9sZCByZXF1ZXN0LgorI2lm ICFBU1NFUlRfRElTQUJMRUQKKyAgICAgICAgR1VuaXF1ZU91dFB0cjxHRXJyb3I+IGVycm9yOwor ICAgICAgICBHUmVmUHRyPEdJbnB1dFN0cmVhbT4gaW5wdXRTdHJlYW0gPSBhZG9wdEdSZWYoc291 cF9yZXF1ZXN0X3NlbmRfZmluaXNoKHNvdXBSZXF1ZXN0LCByZXN1bHQsICZlcnJvci5vdXRQdHIo KSkpOworICAgICAgICBBU1NFUlQoZ19lcnJvcl9tYXRjaGVzKGVycm9yLmdldCgpLCBHX0lPX0VS Uk9SLCBHX0lPX0VSUk9SX0NBTkNFTExFRCkpOworI2VuZGlmCisgICAgICAgIHJldHVybjsKKyAg ICB9CisKICAgICBSZWZQdHI8TmV0d29ya0RhdGFUYXNrU291cD4gcHJvdGVjdGVkVGhpcyA9IGFk b3B0UmVmKHRhc2spOwogICAgIGlmICh0YXNrLT5zdGF0ZSgpID09IFN0YXRlOjpDYW5jZWxpbmcg fHwgdGFzay0+c3RhdGUoKSA9PSBTdGF0ZTo6Q29tcGxldGVkIHx8ICF0YXNrLT5tX2NsaWVudCkg ewogICAgICAgICB0YXNrLT5jbGVhclJlcXVlc3QoKTsKICAgICAgICAgcmV0dXJuOwogICAgIH0K LSAgICBBU1NFUlQoc291cFJlcXVlc3QgPT0gdGFzay0+bV9zb3VwUmVxdWVzdC5nZXQoKSk7CiAK ICAgICBpZiAodGFzay0+c3RhdGUoKSA9PSBTdGF0ZTo6U3VzcGVuZGVkKSB7CiAgICAgICAgIEFT U0VSVCghdGFzay0+bV9wZW5kaW5nUmVzdWx0KTsK