REGRESSION (r138962): Fails to show "confirm form resubmission", hangs browser

• Component: Page Loading
• Status: RESOLVED FIXED

[Adam Barth]

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 [e-mail]
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: [e-mail] [e-mail] 
Labels: Action-BisectNeeded 
Yesterday (16 hours ago) #2 [e-mail]
(No comment was entered for this change.)
Cc: [e-mail] [e-mail] [e-mail] 
Today (15 hours ago) #3 [e-mail]
In my experiments this works correctly in Windows. I have not tried in Linux.
Today (15 hours ago) #4 [e-mail]
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 [e-mail]
The change that caused this issue is http://trac.webkit.org/changeset/138962

[Adam Barth]

Assigned to Alexey to either fix the regression or rollout his change.

[Adam Barth]

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.

[Nate Chapin]

(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.

[Alexey Proskuryakov]

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).

[Alexey Proskuryakov]

I think that the rudeness with which you made your demand is a sign of dirty conscious.

[Maciej Stachowiak]

(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.

[Maciej Stachowiak]

Created attachment 185408 [details]
Patch

[Maciej Stachowiak]

(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.

[Adam Barth]

@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.

[Maciej Stachowiak]

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.

[Adam Barth]

I'm unclear what the current status of this issue is.  @Maciej: Are you still working towards a solution here?

[Maciej Stachowiak]

(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.

[Adam Barth]

> 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.

[Maciej Stachowiak]

(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?

[Adam Barth]

> 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.

[Maciej Stachowiak]

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.

[Maciej Stachowiak]

(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.

[Adam Barth]

> > 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.

[Adam Barth]

s/investing/investigating/

[Adam Barth]

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.

[Adam Barth]

Comment on attachment 185408 [details]
Patch

I'm inclined to land this patch as a short-term workaround.

[WebKit Review Bot]

Comment on attachment 185408 [details]
Patch

Clearing flags on attachment: 185408

Committed r141462: <http://trac.webkit.org/changeset/141462>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

> 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.

[Eric Seidel (no email)]

(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. :)

[Alexey Proskuryakov]

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.

[Maciej Stachowiak]

(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.

[Alexey Proskuryakov]

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.

[Adam Barth]

The behavior is intentional.  Please do not break it.

[Alexey Proskuryakov]

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).