Bug 108214 - REGRESSION (r138962): Fails to show "confirm form resubmission", hangs browser
Summary: REGRESSION (r138962): Fails to show "confirm form resubmission", hangs browser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Maciej Stachowiak
URL:
Keywords:
Depends on:
Blocks: 106147
  Show dependency treegraph
 
Reported: 2013-01-29 12:37 PST by Adam Barth
Modified: 2013-02-04 13:52 PST (History)
10 users (show)

See Also:


Attachments
Patch (1.59 KB, patch)
2013-01-30 00:00 PST, Maciej Stachowiak
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2013-01-29 12:37:36 PST
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
Comment 1 Adam Barth 2013-01-29 12:38:57 PST
Assigned to Alexey to either fix the regression or rollout his change.
Comment 2 Adam Barth 2013-01-29 12:46:07 PST
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.
Comment 3 Nate Chapin 2013-01-29 14:42:12 PST
(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.
Comment 4 Alexey Proskuryakov 2013-01-29 17:31:04 PST
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).
Comment 5 Alexey Proskuryakov 2013-01-29 17:31:45 PST
I think that the rudeness with which you made your demand is a sign of dirty conscious.
Comment 6 Maciej Stachowiak 2013-01-29 18:35:14 PST
(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.
Comment 7 Maciej Stachowiak 2013-01-30 00:00:49 PST
Created attachment 185408 [details]
Patch
Comment 8 Maciej Stachowiak 2013-01-30 00:02:11 PST
(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.
Comment 9 Adam Barth 2013-01-30 00:07:01 PST
@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.
Comment 10 Maciej Stachowiak 2013-01-30 01:45:10 PST
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.
Comment 11 Adam Barth 2013-01-30 16:23:27 PST
I'm unclear what the current status of this issue is.  @Maciej: Are you still working towards a solution here?
Comment 12 Maciej Stachowiak 2013-01-30 22:19:28 PST
(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.
Comment 13 Adam Barth 2013-01-30 22:52:40 PST
> 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.
Comment 14 Maciej Stachowiak 2013-01-30 23:23:01 PST
(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?
Comment 15 Adam Barth 2013-01-30 23:44:25 PST
> 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.
Comment 16 Maciej Stachowiak 2013-01-31 07:38:10 PST
Comment on attachment 185408 [details]
Patch

Flagging for review as a stopgap at Adam's request, but in addition to review I need testing help.
Comment 17 Maciej Stachowiak 2013-01-31 07:44:04 PST
(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.
Comment 18 Adam Barth 2013-01-31 11:45:48 PST
> > 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.
Comment 19 Adam Barth 2013-01-31 11:46:05 PST
s/investing/investigating/
Comment 20 Adam Barth 2013-01-31 11:53:12 PST
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.
Comment 21 Adam Barth 2013-01-31 11:54:24 PST
Comment on attachment 185408 [details]
Patch

I'm inclined to land this patch as a short-term workaround.
Comment 22 WebKit Review Bot 2013-01-31 12:39:52 PST
Comment on attachment 185408 [details]
Patch

Clearing flags on attachment: 185408

Committed r141462: <http://trac.webkit.org/changeset/141462>
Comment 23 WebKit Review Bot 2013-01-31 12:39:57 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 Alexey Proskuryakov 2013-01-31 14:27:15 PST
> 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.
Comment 25 Eric Seidel (no email) 2013-01-31 14:29:41 PST
(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. :)
Comment 26 Alexey Proskuryakov 2013-01-31 14:47:48 PST
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.
Comment 27 Maciej Stachowiak 2013-02-01 10:01:35 PST
(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.
Comment 28 Alexey Proskuryakov 2013-02-01 10:32:49 PST
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.
Comment 29 Adam Barth 2013-02-01 10:42:48 PST
The behavior is intentional.  Please do not break it.
Comment 30 Alexey Proskuryakov 2013-02-01 10:57:20 PST
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).