108214 2013-01-29 12:37:36 -0800 REGRESSION (r138962): Fails to show "confirm form resubmission", hangs browser 2013-02-04 13:52:19 -0800 1 1 1 Unclassified WebKit Page Loading 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P1 Normal --- 106147 1 abarth mjs ap beidson cbiesinger eric fishd fmalita japhet mitz mjs webkit.review.bot oldest_to_newest 819213 0 abarth 2013-01-29 12:37:36 -0800 https://code.google.com/p/chromium/issues/detail?id=172721 What steps will reproduce the problem? 1. Go to a site that that has a form to submit 2. Submit it 3. Go forward 4. Attempt to go back What is the expected output? What do you see instead? You should get an interstitial asking if you want to resubmit the form. On the Mac, the page spins reloading, locking up the io thread and killing the browser. My repro link is an internal Google survey form. Looking for a better repro url. Yesterday (16 hours ago) #1 ligimole@chromium.org Tried this issue using the following link below. https://secure2.sophos.com/en-us/support/contact-support/sample-submission.aspx I could see the tab trying to reload and no browser interaction possible & ultimately need to kill the browser. Build Used : 26.0.1396.1 OS : Mac @mano .. can u try in win & linux as well. Cc: manoranjanr@chromium.org ligimole@chromium.org Labels: Action-BisectNeeded Yesterday (16 hours ago) #2 ligimole@chromium.org (No comment was entered for this change.) Cc: dharani@chromium.org tanyarad@chromium.org anantha@chromium.org Today (15 hours ago) #3 avi@chromium.org In my experiments this works correctly in Windows. I have not tried in Linux. Today (15 hours ago) #4 nyerramilli@chromium.org Tested with the URL mentioned in comment #1, after form submission,click on browser 'back' button --the page spins reloading, locking up the io thread and killing the browser. Able to repro this issue on Win7,Mac book pro 10.8.2 and Ubuntu 12.04 with chrome build 26.0.1396.1 Manual bisect info : Chromium Good build : 26.0.1377.0 (175484) Chromium Bad build : 26.0.1378.0 (175517) You are probably looking for a change made after 175490 (known good), but no lat er than 175498 (first known bad). WEBKIT CHANGELOG URL: http://trac.webkit.org/log/trunk/?rev=139010&stop_rev=138894&verbose=on&limit= 10000 CHANGELOG URL: http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=/tru nk/src&range=175490%3A175498 Labels: -OS-Mac -Action-BisectNeeded OS-All Today (4 hours ago) #5 dharani@chromium.org The change that caused this issue is http://trac.webkit.org/changeset/138962 819216 1 abarth 2013-01-29 12:38:57 -0800 Assigned to Alexey to either fix the regression or rollout his change. 819225 2 abarth 2013-01-29 12:46:07 -0800 This bug is blocking our dev channel release. If you cannot address the issue immediately, please let me know so that I can take further action. 819342 3 japhet 2013-01-29 14:42:12 -0800 (In reply to comment #2) > This bug is blocking our dev channel release. If you cannot address the issue immediately, please let me know so that I can take further action. FWIW, the problem was resolved for me locally if I commented out the new if() block in MainResourceLoader.cpp. 819575 4 ap 2013-01-29 17:31:04 -0800 This is not a fair request. The chromium port had a long-standing technical debt to the WebKit project due to hacking its implementation in a way that only benefitted their platform at the expense of others. I invested the time to properly implement this in a cross-platform manner. This is what chromium developers should have done years ago. The fact that you chose to do an obvious hack instead does not make it others' responsibility to support it. Please don't roll out r138962, other than maybe from your release branch (which I think already happened). 819577 5 ap 2013-01-29 17:31:45 -0800 I think that the rudeness with which you made your demand is a sign of dirty conscious. 819658 6 mjs 2013-01-29 18:35:14 -0800 (In reply to comment #3) > (In reply to comment #2) > > This bug is blocking our dev channel release. If you cannot address the issue immediately, please let me know so that I can take further action. > > FWIW, the problem was resolved for me locally if I commented out the new if() block in MainResourceLoader.cpp. Nate, do you think putting #if !PLATFORM(CHROMIUM) around that block of code is a reasonable short-term fix? I'm going to guess it's not the right long-term solution; it's probably better to make the cross-platform code work right for Chromium as Alexey suggests. But it would let us investigate the right clean solution without the time pressure of a serious regression in the tree. 819857 7 185408 mjs 2013-01-30 00:00:49 -0800 Created attachment 185408 Patch 819858 8 mjs 2013-01-30 00:02:11 -0800 (In reply to comment #7) > Created an attachment (id=185408) [details] > Patch Not flagging for review yet because it would be difficult for me to test or verify broader correctness. I would appreciate input from Chromium folks. 819863 9 abarth 2013-01-30 00:07:01 -0800 @Maciej: Thanks for taking the time to write a patch for this issue. @japhet: Would you be willing to test out Maciej's patch? If not, I can take a look tomorrow when I'm in front of my development machine. 819959 10 mjs 2013-01-30 01:45:10 -0800 After some offline discussion with Alexey and Darin Fisher, I think the patch above may not be the best fix. Probably the root cause is that the fixup code in Chromium's dispatchWillSendRequest is no longer needed, as Alexey's change probably addresses the issue it was trying to work around: <https://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp&exact_package=chromium&q=dispatchWillSendRequest&type=cs%22&l=377> The former comment in Chromium's ResourceHandle::willLoadFromCache alludes to this. If that works, then Chromium would be able to share Alexey's cross-platform solution for handling the interaction of the cache with form resubmission warnings. But it may be more complicated than that. 820773 11 abarth 2013-01-30 16:23:27 -0800 I'm unclear what the current status of this issue is. @Maciej: Are you still working towards a solution here? 821092 12 mjs 2013-01-30 22:19:28 -0800 (In reply to comment #11) > I'm unclear what the current status of this issue is. @Maciej: Are you still working towards a solution here? No, I'm not actively working on this. Offline discussion with Darin Fisher as cited in comment #10 convinced me that the best fix requires a change in Chromium-specific code, of a nature that requires Chromium port expertise. That being said, I'm happy to put up the existing patch for review if it's desirable as a stopgap and if Chromium folks can confirm that it doesn't cause an even worse regression. 821108 13 abarth 2013-01-30 22:52:40 -0800 > That being said, I'm happy to put up the existing patch for review if it's desirable as a stopgap and if Chromium folks can confirm that it doesn't cause an even worse regression. That sounds like a reasonable course of action. Another reasonable course of action is to revert the patch and re-land a revised version once we have verified that it doesn't break other ports. IMHO, the latter course of action is better. 821122 14 mjs 2013-01-30 23:23:01 -0800 (In reply to comment #13) > > That being said, I'm happy to put up the existing patch for review if it's desirable as a stopgap and if Chromium folks can confirm that it doesn't cause an even worse regression. > > That sounds like a reasonable course of action. OK. Have any Chromium folks tried it out? I put it up on EWS for basic testing. > Another reasonable course of action is to revert the patch and re-land a revised version once we have verified that it doesn't break other ports. IMHO, the latter course of action is better. I'd prefer if we could handle this similarly to <https://bugs.webkit.org/show_bug.cgi?id=108380> and <https://bugs.webkit.org/show_bug.cgi?id=52988>. That would indicate folks who understand the port-specific code and folks who understand the patch working together to understand the problem, with revert of an otherwise correct-seeming change being considered a last resort. Is this bug a much more urgent crisis than the other two I cited, or in any other objective way deserving of different treatment? Or can we follow these model examples of dealing with downstream regressions collaboratively? 821143 15 abarth 2013-01-30 23:44:25 -0800 > OK. Have any Chromium folks tried it out? I put it up on EWS for basic testing. I can try it out tomorrow when I'm in front of my development machine. > > Another reasonable course of action is to revert the patch and re-land a revised version once we have verified that it doesn't break other ports. IMHO, the latter course of action is better. > > I'd prefer if we could handle this similarly to <https://bugs.webkit.org/show_bug.cgi?id=108380> and <https://bugs.webkit.org/show_bug.cgi?id=52988>. That would indicate folks who understand the port-specific code and folks who understand the patch working together to understand the problem, with revert of an otherwise correct-seeming change being considered a last resort. Is this bug a much more urgent crisis than the other two I cited, or in any other objective way deserving of different treatment? Or can we follow these model examples of dealing with downstream regressions collaboratively? I would prefer to resolve the issue rather than to debate this topic. 821518 16 185408 mjs 2013-01-31 07:38:10 -0800 Comment on attachment 185408 Patch Flagging for review as a stopgap at Adam's request, but in addition to review I need testing help. 821521 17 mjs 2013-01-31 07:44:04 -0800 (In reply to comment #15) > > OK. Have any Chromium folks tried it out? I put it up on EWS for basic testing. > > I can try it out tomorrow when I'm in front of my development machine. Thanks. If you understand Darin Fisher's suggestion sufficiently, coding that would be super helpful. 821774 18 abarth 2013-01-31 11:45:48 -0800 > > I can try it out tomorrow when I'm in front of my development machine. > > Thanks. The patch does seem to work. The case that's causing the problem is the one where we show an interstitial telling the user that they'll need to resubmit the form. > If you understand Darin Fisher's suggestion sufficiently, coding that would be super helpful. I'm investing the dispatchWillSendRequest approach now. 821776 19 abarth 2013-01-31 11:46:05 -0800 s/investing/investigating/ 821786 20 abarth 2013-01-31 11:53:12 -0800 Removing the mentioned code in FrameLoaderClientImpl::dispatchWillSendRequest does not appear to be the correct approach, at least not without further changes. That results in the form being reposted without user consent. The underlying problem is that Alexey's code assumes that the correct behavior after failing to load a resource with ReturnCacheDataDontLoad is to reload the page. That might be a valid assumption for Safari, but that assumption is not valid of Chrome because Chrome wishes to display an interstitial rather than to reload the page automatically. 821788 21 185408 abarth 2013-01-31 11:54:24 -0800 Comment on attachment 185408 Patch I'm inclined to land this patch as a short-term workaround. 821846 22 185408 webkit.review.bot 2013-01-31 12:39:52 -0800 Comment on attachment 185408 Patch Clearing flags on attachment: 185408 Committed r141462: <http://trac.webkit.org/changeset/141462> 821847 23 webkit.review.bot 2013-01-31 12:39:57 -0800 All reviewed patches have been landed. Closing bug. 821958 24 ap 2013-01-31 14:27:15 -0800 > The underlying problem is that Alexey's code assumes that the correct behavior after failing to load a resource with ReturnCacheDataDontLoad is to reload the page. This is incorrect reading of the code. 821961 25 eric 2013-01-31 14:29:41 -0800 (In reply to comment #24) > > The underlying problem is that Alexey's code assumes that the correct behavior after failing to load a resource with ReturnCacheDataDontLoad is to reload the page. > > This is incorrect reading of the code. Not wishing to be involved... but I can predict the next question someone might ask, which is "Could you help me to read it correctly by explaining a bit more in the bug?" Or maybe it's obvious from the code when they take a second look. Thanks. :) 821982 26 ap 2013-01-31 14:47:48 -0800 Safari does not have the behavior of automatic re-sending a form without a user prompt. Perhaps Adam is referring to a chromium behavior of asking the user twice, but in this case he did not make himself clear. 822907 27 mjs 2013-02-01 10:01:35 -0800 (In reply to comment #26) > Safari does not have the behavior of automatic re-sending a form without a user prompt. > > Perhaps Adam is referring to a chromium behavior of asking the user twice, but in this case he did not make himself clear. Assuming we're talking about behavior for a back/forward navigation to the result of a form submitted with POST in Safari: - If there's an item in the cache, even a stale one, we will use it. - Otherwise we prompt the user before starting the load. - We only do the real network load if the user says yes. I believe prompting is implemented by hooking into a delegate that we send before feeding the load to the network stack. It may be that Chromium hooked up the prompting differently, and just invoking a load from the network level bypasses the prompt logic. 822937 28 ap 2013-02-01 10:32:49 -0800 Testing shipping Chrome behavior, what happens is that an error page is displayed, asking the user to manually reload. When they do, a warning sheet similar to Safari's appears. Unsure if this behavior is intentional, a bug, or a stop-gap solution because of implementation complexity (the complexity is now removed). It does not look right to me personally. 822945 29 abarth 2013-02-01 10:42:48 -0800 The behavior is intentional. Please do not break it. 822973 30 ap 2013-02-01 10:57:20 -0800 OK. One way to prevent such things from happening in the future is by implementing non-default behaviors via specific delegates, and by discussing it with the WebKit community whether such behaviors are something WebKit should support. It puts an undue burden on the community when functions with specific clear purpose are hacked to do something different, without even explaining the rationale (which remains a mystery). 185408 2013-01-30 00:00:49 -0800 2013-01-31 12:39:52 -0800 Patch bug-108214-20130129235733.patch text/plain 1631 mjs SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE0MTIyOSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDEzLTAxLTI5ICBNYWNpZWog U3RhY2hvd2lhayAgPG1qc0BhcHBsZS5jb20+CisKKyAgICAgICAgUkVHUkVTU0lPTiAocjEzODk2 Mik6IEZhaWxzIHRvIHNob3cgImNvbmZpcm0gZm9ybSByZXN1Ym1pc3Npb24iLCBoYW5ncyBicm93 c2VyCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDgy MTQKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKyAgICAgICAgCisgICAg ICAgIEF0dGVtcHQgYSB3b3JrYXJvdW5kIHN1Z2dlc3RlZCBieSBOYXRlIENoYXBpbiBieSBwdXR0 aW5nIHNvbWUgY29kZQorICAgICAgICBpbiAjaWZkZWYgIVBMQVRGT1JNKENIUk9NSVVNKS4KKwor ICAgICAgICAqIGxvYWRlci9NYWluUmVzb3VyY2VMb2FkZXIuY3BwOgorICAgICAgICAoV2ViQ29y ZTo6TWFpblJlc291cmNlTG9hZGVyOjpub3RpZnlGaW5pc2hlZCk6CisKIDIwMTMtMDEtMjkgIFNo aW55YSBLYXdhbmFrYSAgPHNoaW55YWtAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFJlbmRlcmVy IGlzIHJlY3JlYXRlZCB1bmV4cGVjdGVkbHkgYWZ0ZXIgZGV0YWNoIGluIEhUTUxJbnB1dEVsZW1l bnQKSW5kZXg6IFNvdXJjZS9XZWJDb3JlL2xvYWRlci9NYWluUmVzb3VyY2VMb2FkZXIuY3BwCj09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2xvYWRlci9NYWluUmVzb3VyY2VMb2FkZXIuY3Bw CShyZXZpc2lvbiAxNDEyMjgpCisrKyBTb3VyY2UvV2ViQ29yZS9sb2FkZXIvTWFpblJlc291cmNl TG9hZGVyLmNwcAkod29ya2luZyBjb3B5KQpAQCAtNTc2LDEwICs1NzYsMTMgQEAgdm9pZCBNYWlu UmVzb3VyY2VMb2FkZXI6Om5vdGlmeUZpbmlzaGVkKAogICAgICAgICByZXR1cm47CiAgICAgfQog CisgICAgLy8gRklYTUU6IHdlIHNob3VsZCBmaXggdGhlIGRlc2lnbiB0byBlbGltaW5hdGUgdGhl IG5lZWQgZm9yIGEgcGxhdGZvcm0gaWZkZWYgaGVyZQorI2lmICFQTEFURk9STShDSFJPTUlVTSkK ICAgICBpZiAobV9kb2N1bWVudExvYWRlci0+cmVxdWVzdCgpLmNhY2hlUG9saWN5KCkgPT0gUmV0 dXJuQ2FjaGVEYXRhRG9udExvYWQgJiYgIW1fcmVzb3VyY2UtPndhc0NhbmNlbGVkKCkpIHsKICAg ICAgICAgZnJhbWVMb2FkZXIoKS0+cmV0cnlBZnRlckZhaWxlZENhY2hlT25seU1haW5SZXNvdXJj ZUxvYWQoKTsKICAgICAgICAgcmV0dXJuOwogICAgIH0KKyNlbmRpZgogCiAgICAgY29uc3QgUmVz b3VyY2VFcnJvciYgZXJyb3IgPSBtX3Jlc291cmNlLT5yZXNvdXJjZUVycm9yKCk7CiAgICAgaWYg KGRvY3VtZW50TG9hZGVyKCktPmFwcGxpY2F0aW9uQ2FjaGVIb3N0KCktPm1heWJlTG9hZEZhbGxi YWNrRm9yTWFpbkVycm9yKHJlcXVlc3QoKSwgZXJyb3IpKQo=