392567 - Impossible to submit forms to JAR URLs

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Wladimir Palant]

Attachment: JAR file for testcase

If I try to submit a GET form to a JAR URL the browser simply requests the form's action URI instead of attaching query parameters to it. Reason seems to be that nsFormSubmission uses nsIURI::SetPath that is not implemented by nsJARURI.

Comment 1

[Wladimir Palant]

Attachment: Testcase

Open this testcase, enter something into the text field and submit the form. The browser will open jar:https://bugzilla.mozilla.org/attachment.cgi?id=277073!/index.html instead of the expected jar:https://bugzilla.mozilla.org/attachment.cgi?id=277073!/index.html?test=some_text.

This example also shows that simply implementing SetPath in nsJARURI will not help, this URL already contains one query string and form submission should not change it. Instead, form submission should use GetFilePath and SetFilePath methods (if nsIURL is implemented).

Comment 2

[Wladimir Palant]

Attachment: Proposed patch

Actually, it is better to use nsIURL::SetQuery().

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 277077 [details] [diff] [review]
Proposed patch

This is wrong, because if there is an existing query it doesn't remove it.  I expect an automated regression test for this in the next patch version.

Why not just do a single SetQuery call when this is an nsIURL and do this rigmarole otherwise?

That said, I was sure we'd tried that once and it broke something, so if you go that route test well!

And perhaps jar URIs should implement SetPath?  Not sure what form it would take, though, since there are two paths involved.

Comment 4

[Wladimir Palant]

Attachment: Patch v2

Attaching new patch - now using nsIURL::SetQuery() whenever possible without even trying all the tricks meant for nsIURI. Also added a testcase and checked all existing mochitest testcases (no issues found). Will attach -w patch to make reviewing easier.

Comment 5

[Wladimir Palant]

Attachment: Patch v2 with -w

Comment 6

[Boris Zbarsky [:bzbarsky]]

> checked all existing mochitest testcases

We don't really have any form submission tests yet....

In any case, the bug I was thinking of was bug 185169, so you should be OK here, I think.  But please keep an eye on incoming bugs for a few weeks and watch out for form submission issues.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 277373 [details] [diff] [review]
Patch v2 with -w

>Index: content/html/content/src/nsFormSubmission.cpp
>     nsCAutoString path;
>+
>     rv = aURI->GetPath(path);

Why add that blank line?

>Index: content/html/content/test/test_bug392567.html

>+  is(frame.location.href, expected, "Submitting to " + tests[currentTest][0] + ", expecting to see URL " + expected + " - seeing " + frame.location.href);

If this fails, |is| will report what it saw and what was expected.  So I'd make the message just: "Submitting to " + tests[currentTest][0]

r=bzbarsky with those nits picked.

Comment 8

[Wladimir Palant]

Attachment: Patch v3

Comment 9

[Wladimir Palant]

Attachment: Patch v3 with -w

Comment 10

[Wladimir Palant]

Comment on attachment 277508 [details] [diff] [review]
Patch v3

Requesting approval for 1.9: This fixes a bug that prevents using jar: urls like regular http: or file: URLs. In my case this was necessary to distribute lots of help files in one single archive, something that I think is required rather often.

Comment 11

[Phil Ringnalda (:philor)]

Doesn't seem to be anything checkin-needed yet, if you're still waiting on approval.

Comment 12

[Reed Loden [:reed]]

Checking in content/html/content/src/nsFormSubmission.cpp;
/cvsroot/mozilla/content/html/content/src/nsFormSubmission.cpp,v  <--  nsFormSubmission.cpp
new revision: 1.60; previous revision: 1.59
done
Checking in content/html/content/test/Makefile.in;
/cvsroot/mozilla/content/html/content/test/Makefile.in,v  <--  Makefile.in
new revision: 1.28; previous revision: 1.27
done
RCS file: /cvsroot/mozilla/content/html/content/test/test_bug392567.html,v
done
Checking in content/html/content/test/test_bug392567.html;
/cvsroot/mozilla/content/html/content/test/test_bug392567.html,v  <--  test_bug392567.html
initial revision: 1.1
done