Bug 93850

Summary: FrameLoader::receivedMainResourceError doesn't handle GET cancellations well.
Product: WebKit Reporter: Mike West <mkwst>
Component: WebCore Misc.Assignee: Mike West <mkwst>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, japhet, jochen, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 93777    
Attachments:
Description Flags
Patch none

Mike West
Reported 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.
Attachments
Patch (2.28 KB, patch)
2012-08-13 08:23 PDT, Mike West 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Mike West
Comment 1 2012-08-13 08:23:56 PDT
Created attachment 158005 [details] Patch
Mike West
Comment 2 2012-08-13 08:24:17 PDT
WDYT, Jochen?
jochen
Comment 3 2012-08-13 08:39:13 PDT
I'd defer to Adam or Nate on this
Mike West
Comment 4 2012-08-13 08:43:42 PDT
Cool. Ideas for a test would be helpful. :)
Adam Barth
Comment 5 2012-08-13 10:43:28 PDT
Makes sense to me, but I'd like to hear from Nate before landing.
Mike West
Comment 6 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.
Nate Chapin
Comment 7 2012-08-13 11:04:03 PDT
Comment on attachment 158005 [details] Patch LGTM, but no test?
Mike West
Comment 8 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.
Adam Barth
Comment 9 2012-08-13 11:33:46 PDT
Presumably we'll get a test as part of Bug 93777?
Mike West
Comment 10 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!" :)
Adam Barth
Comment 11 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. :)
Mike West
Comment 12 2012-08-13 11:56:59 PDT
Comment on attachment 158005 [details] Patch In that case, CQ?
WebKit Review Bot
Comment 13 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>
WebKit Review Bot
Comment 14 2012-08-13 12:30:34 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.