429347 - URL parser gets confused by ref with no path

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Parsing "http://example.com#bar" produces an nsStandardURL with an mBasename segment with pos == 0, len == -1.  Parsing "http://example.com/#bar" gives an mBasename segment with pos == 18, len == 0.

The resulting URI objects serialize to the same thing, but don't test equal via equals().  This is wrong.  See bug 429128 comment 12 and bug 429128 comment 14.

Comment 1

[Michal Novotny [:michal]]

Attachment: fix + unit test

Comment 2

[Boris Zbarsky [:bzbarsky]]

Wouldn't it be better to fix ParsePath?  I guess that pretty purposefully does the -1 thing....

If we do want to do this, wouldn't it make more sense to adjust the filename len first, _then_ do the > 0 compare and ensuing block?

Comment 3

[Michal Novotny [:michal]]

Attachment: patch v2

fixed nsBaseURLParser::ParsePath()

Comment 4

[Boris Zbarsky [:bzbarsky]]

So with that patch, how do we parse "http://example.com" as compared to "http://example.com/"?  Do both end up with mBasename of len == -1?  Or len == 0?  Or something else?

Comment 5

[Michal Novotny [:michal]]

This isn't affected by this patch and both are mBasename = {mPos = 19, mLen = 0}.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 340919 [details] [diff] [review]
patch v2

>Index: netwerk/base/src/nsURLParsers.cpp
>+    // an empty file path is no file path only if there is neither param/query/ref

s/neither/no/

>Index: netwerk/test/unit/test_bug429347.js

Add as a test the thing I asked about with no query/param/ref?

I still don't see how that works; do we never even hit this function in that case?  Depending on that seems pretty fragile...

r=bzbarsky with that satisfactorily explained.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh, I'd like biesi to ok this too.

Comment 8

[Michal Novotny [:michal]]

Attachment: patch v3

I think I've found a better place to fix this bug. Please have a look at the new patch.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 341035 [details] [diff] [review]
patch v3

This also looks good to me (and still needs a glance from biesi)

Comment 10

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

Comment on attachment 341035 [details] [diff] [review]
patch v3

You can reenable the line in netwerk/test/unit/test_standardurl.js now, right?

Comment 11

[Michal Novotny [:michal]]

Attachment: patch v4

(In reply to comment #10)
> (From update of attachment 341035 [details] [diff] [review])
> You can reenable the line in netwerk/test/unit/test_standardurl.js now, right?

You're right.

Comment 12

[Reed Loden [:reed]]

http://hg.mozilla.org/mozilla-central/rev/3fcf14282433

Comment 13

[Julian Reschke]

Note that a fix for 665706 will affect a test case over here:

  // see https://bugzilla.mozilla.org/show_bug.cgi?id=665706
  // ";" is not parsed as special anymore and thus ends up
  // in the authority component (see RFC 3986)
  uri1.spec = "http://example.com;bar";
  uri2.spec = "http://example.com/;bar";
  do_check_false(uri1.equals(uri2));

...so these two intentionally do not compare equal anymore.