145242 2015-05-20 19:27:00 -0700 dispatchViewStateChange should not wait for sync reply if the page isn't visible 2015-05-22 09:54:26 -0700 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 barraclough barraclough ap thorton oldest_to_newest 1096457 0 barraclough 2015-05-20 19:27:00 -0700 This is particularly problematic on iOS, since if the page isn't visible the process is likely suspended. <rdar://problem/20967937> 1096459 1 253496 barraclough 2015-05-20 19:30:16 -0700 Created attachment 253496 Fix 1096463 2 barraclough 2015-05-20 19:37:01 -0700 Fixed in r184692 1096793 3 253496 ap 2015-05-21 23:20:17 -0700 Comment on attachment 253496 Fix View in context: https://bugs.webkit.org/attachment.cgi?id=253496&action=review > Source/WebKit2/UIProcess/WebPageProxy.cpp:1377 > + if (!(m_viewState & ViewState::IsVisible)) > + m_viewStateChangeWantsSynchronousReply = false; The initial m_viewState is invisible, so doesn't this patch change behavior even for pages that are actually visible? This seems very dangerous. 1096810 4 barraclough 2015-05-22 00:46:49 -0700 (In reply to comment #3) > Comment on attachment 253496 [details] > Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=253496&action=review > > > Source/WebKit2/UIProcess/WebPageProxy.cpp:1377 > > + if (!(m_viewState & ViewState::IsVisible)) > > + m_viewStateChangeWantsSynchronousReply = false; > > The initial m_viewState is invisible, so doesn't this patch change behavior > even for pages that are actually visible? I believe we were only doing the sync wait when the app reactivated, in which case the visible view was already in the hierarchy & marked visible, so the sync update isn't suppressed. > This seems very dangerous. We shouldn't ever need to synchronously change the view state while not visible. If the act on changing the view from invisible to visible needed to be synchronous, then that call to set the view state needed to be the one marked to be synchronous. If we were ever failing to do so, then that was a bug that could cause problems; we should find it and fix it. This is bad behavior, and we shouldn't keep it to try to paper over other hypothetical mistakes. 1096878 5 ap 2015-05-22 09:54:26 -0700 > I believe we were only doing the sync wait when the app reactivated, in which case the visible view was already in the hierarchy & marked visible, so the sync update isn't suppressed. Ok. > we should find it and fix it What I'm saying is that this change is more dangerous than it seems, so it should be tested proactively and thoroughly. There is very little difference between uncovering a pre-existing bug, and creating a new one. 253496 2015-05-20 19:30:16 -0700 2015-05-20 19:33:26 -0700 Fix 145242.1.patch text/plain 2388 barraclough SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDE4NDY5MCkKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBACisyMDE1LTA1LTIwICBHYXZpbiBC YXJyYWNsb3VnaCAgPGJhcnJhY2xvdWdoQGFwcGxlLmNvbT4KKworICAgICAgICBkaXNwYXRjaFZp ZXdTdGF0ZUNoYW5nZSBzaG91bGQgbm90IHdhaXQgZm9yIHN5bmMgcmVwbHkgaWYgdGhlIHBhZ2Ug aXNuJ3QgdmlzaWJsZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9MTQ1MjQyCisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS8yMDk2NzkzNz4KKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaGlzIGlzIHBhcnRpY3Vs YXJseSBwcm9ibGVtYXRpYyBvbiBpT1MsIHNpbmNlIGlmIHRoZSBwYWdlIGlzbid0IHZpc2libGUg dGhlIHByb2Nlc3MgaXMgbGlrZWx5IHN1c3BlbmRlZC4KKworCisgICAgICAgICogVUlQcm9jZXNz L1dlYlBhZ2VQcm94eS5jcHA6CisgICAgICAgIChXZWJLaXQ6OldlYlBhZ2VQcm94eTo6ZGlzcGF0 Y2hWaWV3U3RhdGVDaGFuZ2UpOgorICAgICAgICAoV2ViS2l0OjpXZWJQYWdlUHJveHk6OndhaXRG b3JEaWRVcGRhdGVWaWV3U3RhdGUpOgorCiAyMDE1LTA1LTIwICBNYXJjb3MgQ2hhdmFycsOtYSBU ZWlqZWlybyAgPG1jaGF2YXJyaWFAaWdhbGlhLmNvbT4KIAogICAgICAgICBFbmFibGUgZGlzayBj YWNoZSBmb3IgcmFuZ2UgcmVxdWVzdHMKSW5kZXg6IFNvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9X ZWJQYWdlUHJveHkuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9X ZWJQYWdlUHJveHkuY3BwCShyZXZpc2lvbiAxODQ2MTQpCisrKyBTb3VyY2UvV2ViS2l0Mi9VSVBy b2Nlc3MvV2ViUGFnZVByb3h5LmNwcAkod29ya2luZyBjb3B5KQpAQCAtMTM3Miw2ICsxMzcyLDEw IEBAIHZvaWQgV2ViUGFnZVByb3h5OjpkaXNwYXRjaFZpZXdTdGF0ZUNoYW4KICAgICBpZiAobV92 aWV3V2FzRXZlckluV2luZG93ICYmIChjaGFuZ2VkICYgVmlld1N0YXRlOjpJc0luV2luZG93KSAm JiBpc0luV2luZG93KCkpCiAgICAgICAgIG1fdmlld1N0YXRlQ2hhbmdlV2FudHNTeW5jaHJvbm91 c1JlcGx5ID0gdHJ1ZTsKIAorICAgIC8vIERvbid0IHdhaXQgc3luY2hyb25vdXNseSBpZiB0aGUg dmlldyBzdGF0ZSBpcyBub3QgdmlzaWJsZS4gKFRoaXMgbWF0dGVycyBpbiBwYXJ0aWN1bGFyIG9u IGlPUywgd2hlcmUgYSBoaWRkZW4gcGFnZSBtYXkgYmUgc3VzcGVuZGVkLikKKyAgICBpZiAoISht X3ZpZXdTdGF0ZSAmIFZpZXdTdGF0ZTo6SXNWaXNpYmxlKSkKKyAgICAgICAgbV92aWV3U3RhdGVD aGFuZ2VXYW50c1N5bmNocm9ub3VzUmVwbHkgPSBmYWxzZTsKKwogICAgIGlmIChjaGFuZ2VkIHx8 IG1fdmlld1N0YXRlQ2hhbmdlV2FudHNTeW5jaHJvbm91c1JlcGx5IHx8ICFtX25leHRWaWV3U3Rh dGVDaGFuZ2VDYWxsYmFja3MuaXNFbXB0eSgpKQogICAgICAgICBtX3Byb2Nlc3MtPnNlbmQoTWVz c2FnZXM6OldlYlBhZ2U6OlNldFZpZXdTdGF0ZShtX3ZpZXdTdGF0ZSwgbV92aWV3U3RhdGVDaGFu Z2VXYW50c1N5bmNocm9ub3VzUmVwbHksIG1fbmV4dFZpZXdTdGF0ZUNoYW5nZUNhbGxiYWNrcyks IG1fcGFnZUlEKTsKIApAQCAtMTQ1MCw2ICsxNDU0LDE1IEBAIHZvaWQgV2ViUGFnZVByb3h5Ojp3 YWl0Rm9yRGlkVXBkYXRlVmlld1MKICAgICBpZiAobV93YWl0aW5nRm9yRGlkVXBkYXRlVmlld1N0 YXRlKQogICAgICAgICByZXR1cm47CiAKKyNpZiBQTEFURk9STShJT1MpCisgICAgLy8gSGFpbCBN YXJ5IGNoZWNrLiBTaG91bGQgbm90IGJlIHBvc3NpYmxlIChkaXNwYXRjaFZpZXdTdGF0ZUNoYW5n ZSBzaG91bGQgZm9yY2UgYXN5bmMgaWYgbm90IHZpc2libGUsCisgICAgLy8gYW5kIGlmIHZpc2li bGUgd2Ugc2hvdWxkIGJlIGhvbGRpbmcgYW4gYXNzZXJ0aW9uKSAtIGJ1dCB3ZSBzaG91bGQgbmV2 ZXIgYmxvY2sgb24gYSBzdXNwZW5kZWQgcHJvY2Vzcy4KKyAgICBpZiAoIW1fYWN0aXZpdHlUb2tl bikgeworICAgICAgICBBU1NFUlRfTk9UX1JFQUNIRUQoKTsKKyAgICAgICAgcmV0dXJuOworICAg IH0KKyNlbmRpZgorCiAgICAgbV93YWl0aW5nRm9yRGlkVXBkYXRlVmlld1N0YXRlID0gdHJ1ZTsK IAogICAgIG1fZHJhd2luZ0FyZWEtPndhaXRGb3JEaWRVcGRhdGVWaWV3U3RhdGUoKTsK