Bug 93850 - FrameLoader::receivedMainResourceError doesn't handle GET cancellations well.
Summary: FrameLoader::receivedMainResourceError doesn't handle GET cancellations well.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike West
URL:
Keywords:
Depends on:
Blocks: 93777
  Show dependency treegraph
 
Reported: 2012-08-13 08:15 PDT by Mike West
Modified: 2012-08-13 12:30 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.28 KB, patch)
2012-08-13 08:23 PDT, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike West 2012-08-13 08:15:46 PDT
If a form submission via GET is cancelled, FrameLoader::receivedMainResourceError fails to clear m_submittedFormURL, as the form's action doesn't match the requested URL.
Comment 1 Mike West 2012-08-13 08:23:56 PDT
Created attachment 158005 [details]
Patch
Comment 2 Mike West 2012-08-13 08:24:17 PDT
WDYT, Jochen?
Comment 3 jochen 2012-08-13 08:39:13 PDT
I'd defer to Adam or Nate on this
Comment 4 Mike West 2012-08-13 08:43:42 PDT
Cool. Ideas for a test would be helpful. :)
Comment 5 Adam Barth 2012-08-13 10:43:28 PDT
Makes sense to me, but I'd like to hear from Nate before landing.
Comment 6 Mike West 2012-08-13 10:49:19 PDT
(In reply to comment #5)
> Makes sense to me, but I'd like to hear from Nate before landing.

For context, http://trac.webkit.org/browser/trunk/Source/WebCore/loader/FrameLoader.cpp#L2571 is the line where it breaks down now. The URLs don't match, as the latter has the whole query string.
Comment 7 Nate Chapin 2012-08-13 11:04:03 PDT
Comment on attachment 158005 [details]
Patch

LGTM, but no test?
Comment 8 Mike West 2012-08-13 11:23:00 PDT
(In reply to comment #7)
> (From update of attachment 158005 [details])
> LGTM, but no test?

Yes, it should have a test before landing it. I talked with Jochen about it, and neither of us could quickly come up with a test that would trigger the conditions. He noted there was a method on testRunner to make the next request fail, but didn't think it would work in the context of a form submit.

I'd like to have a test here, I'd appreciate your input as to how it might be structured.
Comment 9 Adam Barth 2012-08-13 11:33:46 PDT
Presumably we'll get a test as part of Bug 93777?
Comment 10 Mike West 2012-08-13 11:36:26 PDT
(In reply to comment #9)
> Presumably we'll get a test as part of Bug 93777?

Sure. You'll get two, in fact!

But that's somewhat of a cop out, isn't it? "Dude, I totally have a test in the next patch. I swear!" :)
Comment 11 Adam Barth 2012-08-13 11:52:17 PDT
Comment on attachment 158005 [details]
Patch

Yeah, but given that the next patch is already written and the tests are there, I don't think there's that much risk that you won't come through.  :)
Comment 12 Mike West 2012-08-13 11:56:59 PDT
Comment on attachment 158005 [details]
Patch

In that case, CQ?
Comment 13 WebKit Review Bot 2012-08-13 12:30:30 PDT
Comment on attachment 158005 [details]
Patch

Clearing flags on attachment: 158005

Committed r125436: <http://trac.webkit.org/changeset/125436>
Comment 14 WebKit Review Bot 2012-08-13 12:30:34 PDT
All reviewed patches have been landed.  Closing bug.