429128 - Relative URI resolution broken in some cases

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

js> base = ios.newURI("http://www.example.com/#", null, null)
[xpconnect wrapped nsIURI]
js> base.nsIURL.query = "param=value"
param=value
js> base.spec
http://www.example.com/?param=value#
js> ios.newURI("#ref", null, base).spec
http://www.example.com/?#ref

Note that the query parameters are gone. They should not be. This was working correctly in Firefox 2.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Well actually this seems to be broken also in Firefox 2, but form submission doesn't seem to hit the bug there (it does on trunk)

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

(bug 392567 changed form submission to expose this bug)

Comment 3

[Benjamin Smedberg]

Oh man, that seems pretty serious. I think we should fix this at least in the form-submission case, if not in general.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Unfortunately I can't test this until Friday at the earliest because I don't have a build environment here.

Comment 5

[Mike Schroepfer]

Michal can you jump in an verify this patch?

Comment 6

[Michal Novotny [:michal]]

(In reply to comment #5)
> Michal can you jump in an verify this patch?

Yes, I'll post result in few minutes.

Comment 7

[Michal Novotny [:michal]]

I can confirm that the patch works. Output when patch is applied:

js> var ios = Components.classes["@mozilla.org/network/io-service;1"].
            getService(Components.interfaces.nsIIOService);
js> var base = ios.newURI("http://www.example.com/#", null, null);
js> base.nsIURL.query = "param=value"
param=value
js> base.spec
http://www.example.com/?param=value#
js> ios.newURI("#ref", null, base).spec
http://www.example.com/?param=value#ref

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 315772 [details] [diff] [review]
patch

Did a tryserver build; this does fix the issue on the website I was seeing it on.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: unit test

for putting into netwerk/test/unit

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 315781 [details]
unit test

(note to self: fix the BUGID in this file)

Comment 11

[Boris Zbarsky [:bzbarsky]]

For what it's worth, I have a slightly more extensive unit test for this, one which caught another bug in nsStandardURL, as far as I can tell.  I'll post what I have.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: More test

The nsStandardURL change was needed because otherwise this case:

  ["http://example.com#bar", "http://example.com/?foo#bar"],

failed the equals() check, because the first URI had an mBasename segment with pos == 0, len == -1, while the second one had pos == 10, len == 0.  I think those two segments should test true...  If that's wrong, we should disble this one line, and fix the url parser to not produce a len == -1 basename, I guess.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 315772 [details] [diff] [review]
patch

This looks good.  See my test?

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #12)
>   ["http://example.com#bar", "http://example.com/?foo#bar"],

I think it would be better to ensure that the mBasename fields are identical when the URIs have the same spec.

Comment 15

[Damon Sicore (:damons)]

This seems pretty serious.  +'ing.  

Comment 16

[Boris Zbarsky [:bzbarsky]]

So... that would involve changing the urlparser bits or something.  Not quite sure what.  I'm certainly not in a position to do that safely at this point in a release (just don't have the time to learn the code).

So let's say we comment out that one line of test and file a followup bug on it, and I guess not make that nsStandardURL change?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Add a couple more tests

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 315903 [details] [diff] [review]
Add a couple more tests

sounds good

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 315903 [details] [diff] [review]
Add a couple more tests

a=beltzner, but you don't need approvals for tests.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 315772 [details] [diff] [review]
patch

a=beltzner, oh, sure, have approval for the patch, as well :)

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Final test

I filed bug 429347 on the "com#bar" thing.

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in netwerk/base/src/nsStandardURL.cpp;
/cvsroot/mozilla/netwerk/base/src/nsStandardURL.cpp,v  <--  nsStandardURL.cpp
new revision: 1.110; previous revision: 1.109
done
Checking in netwerk/test/unit/test_standardurl.js;
/cvsroot/mozilla/netwerk/test/unit/test_standardurl.js,v  <--  test_standardurl.js
new revision: 1.2; previous revision: 1.1
done

Comment 23

[John P Baker]

Presumably, although I haven't tried too hard to untangle it, this was also bug 426332.