225910 - Remove hand-parsing for refs in content sinks and docshell

Status: RESOLVED FIXED

(Core :: Networking, defect, P5)

Description

[Boris Zbarsky [:bzbarsky]]

The content sinks and docshell do this hand-parsing of refs... Now that nsJARURI
implements nsIURL, I do not think that's necessary any longer.  I propose to
remove that code.

Refs:

http://lxr.mozilla.org/seamonkey/source/content/html/document/src/nsHTMLContentSink.cpp#3811
http://lxr.mozilla.org/seamonkey/source/content/xml/document/src/nsXMLContentSink.cpp#883
http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#5702

I think I can use getRelativeSpec to simplify the docshell code (if the relative
spec of aURI relative to mCurrentURI starts with a '#', then it just differs by
an anchor).

Thoughts?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Alright, looks like getRelativeSpec will in fact not help me one whit here.  I
may have to temporarily set the refs on mCurrentURI and aURI to nothing,
compare, then unset them back...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Something like this

This should also resolve any case-sensitivity issues we had there...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 135700 [details] [diff] [review]
Something like this

darin, what do you think?

Comment 4

[Harshal Pradhan]

Oops ... sorry. I didnt intend to clobber the priority and milestone.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Actually, what I'd really love is if someone with access to a reasonable Tp
setup could run the Tp tests on this patch... that would indicate whether we
could in fact use something like this or whether I should just leave the
image-specific fix I put in and be content.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Er, ignore comment 5.  Wrong bug.  ;)

Comment 7

[Darin Fisher]

i presume we do not care about scrolling to named anchors in content generated
from a data: URL or even a wyciwyg: URI?  hmm... wyciwyg could be a real issue, no?

Comment 8

[Boris Zbarsky [:bzbarsky]]

For a data: uri, I think that would be incorrect behavior, plain and simple. See
URL field of this bug.

You're right about wyciwyg.  Is there any call to make that an nsIURL?

Comment 9

[Darin Fisher]

no, unfortunately wyciwyg just uses nsSimpleURI.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I realize that. The question is whether it should.  ;)

Comment 11

[Darin Fisher]

spoke with jst a bit about this... it seems like it might make sense for
nsSimpleURI to implement nsIURL.  in fact, it seems like this whole distinction
between what a URI is and what a URL is is just pretty bogus.  RFC 2396 says
that all URIs should support reference fragments.  section 2.4.3 says this:

  The character "#" is [...] used to delimit a URI from a fragment identifier in
  URI references.

perhaps we can make nsSimpleURI simply not implement the other nsIURL methods. 
this would be consistent with the way nsSimpleURI implements GetHost and SetHost
for example.

it's a shame nsIURI doesn't have the Get/SetRef methods.

btw, what makes you think that data: URIs cannot have a reference fragment?  RFC
2396 says that '#' is a character used as a URI delimiter (part of the "delims"
set).  that implies that it should be escaped if included in the URi itself.

jst also mentioned that he has seen some pages that depend on javascript: URIs
supporting reference fragments.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> it's a shame nsIURI doesn't have the Get/SetRef methods.

Yeah.  :(

> btw, what makes you think that data: URIs cannot have a reference fragment? 

Well, either they can't or our rendering of that testcase in the URL field is
buggy... ;)

> jst also mentioned that he has seen some pages that depend on javascript: URIs
> supporting reference fragments.

Oh, lord...  OK.  Perhaps we should in fact implement nsIURL on nsSimpleURI...
That will involve a sweep of the tree for people who assume that once you QI to
nsIURL you can get things like paths with impunity and no error returns... :(

Comment 13

[Boris Zbarsky [:bzbarsky]]

Perhaps we should just wontfix this, btw, with nice comments explaining why
people shouldn't bother....

Comment 14

[Boris Zbarsky [:bzbarsky]]

Note that the "something like this" patch breaks <a href=#"> -- it treats it as
not having a ref at all.

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to comment #12)
> > it's a shame nsIURI doesn't have the Get/SetRef methods.
> 
> Yeah.  :(

Now they do! :) (as of bug 308590 landing)

(In reply to comment #11)
> jst also mentioned that he has seen some pages that depend on
> javascript: URIs supporting reference fragments.

They also support Get/SetRef, as of bug 308590 & bug 659698!

I don't know if there's anything that remains to be done here (since this bug has been mostly untouched for 6 years), but perhaps we can use nsIURI::Get/SetRef methods to fix this now?

(FWIW, I stumbled across this bug due to a comment inside of nsContentUtils::SplitURIAtHash mentioning the bug #)

Comment 16

[Boris Zbarsky [:bzbarsky]]

We can't quite use GetRef/SetRef, sadly.  The docshell code needs to be able to tell apart URIs ending in just '#' with nothing after it and URIs with no ref at all, and nsIURI does not expose that difference.  :(

But yes, the docshell code is still doing hand-parsing of IRIs, and I would still love to get rid of it.

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to comment #16)
> We can't quite use GetRef/SetRef, sadly.  The docshell code needs to be able
> to tell apart URIs ending in just '#' with nothing after it and URIs with no
> ref at all, and nsIURI does not expose that difference.  :(

We can tell that pretty easily by inspecting the last character of GetSpec() in that case, though.

Perhaps a helper-method "nsContentUtils::CompareURIs()" that returns an enum value to distinguish base-uri-differs vs. just-the-ref-differs  (vs. uris-exactly-equal) would be useful here?  (see end of bug 662094 comment 1 for more)

Comment 18

[Dragana Damjanovic [:dragana]]

In the current code, this parsing is only done in nsDocShell. I could not find any in parsing in nsHTMLContentSink.cpp or nsXMLContentSink.cpp.

Only use is at:
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9982

nsContentUtils::SplitURIAtHash is used now and I am not convinced that changing that to GetRef/GetHasRef would be faster or nicer (ok, we do noth need to parse it multiple times for a single uri). 

Boris, should we change this?

Comment 19

[Boris Zbarsky [:bzbarsky]]

See comment 16 for the issue with GetRef here.  Does GetHasRef let us distinguish between "http://foo" and "http://foo#"?

Comment 20

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=071b9b5b8eac

Comment 21

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b83ce3908677

Comment 22

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=096c7576a37b

Comment 23

[Dragana Damjanovic [:dragana]]

(In reply to Boris Zbarsky [:bz] from comment #19)
> See comment 16 for the issue with GetRef here.  Does GetHasRef let us
> distinguish between "http://foo" and "http://foo#"?

Yes, for http://foo# hasRef is true and for http://foo it is false

Comment 24

[Dragana Damjanovic [:dragana]]

Attachment: bug_225910_v1.patch

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8735117 [details] [diff] [review]
bug_225910_v1.patch

Please don't ignore failure returns from GetRef/GetHasRef/EqualsExceptRef.  The old code is careful about not assuming nsIURI methods succeed, and even has an explicit comment to that effect.  If those fail, set sameExceptHashes to false.

I'd like to see an updated patch with that fixed.

>+                            ((curURIHasRef != newURIHasRef) || !curHash.Equals(newHash));

That first != is over-parenthesized.

The rest looks good; I hope we have good enough test coverage here and fear we might not...

Note that it seems like we could fix nsGlobalWindow::DispatchAsyncHashchange to do something similar as well, and nix SplitURIAtHash completely.

Comment 26

[Dragana Damjanovic [:dragana]]

Attachment: bug_225910_v2.patch

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8736214 [details] [diff] [review]
bug_225910_v2.patch

>+    nsresult rvURIOld;

Could you please push this down into the two blocks where it's actually used, so it won't be visible in a scope where it might not even be initialized?

> nsGlobalWindow::DispatchAsyncHashchange(nsIURI *aOldURI, nsIURI *aNewURI)

>+  NS_ENSURE_STATE(NS_SUCCEEDED(aOldURI->GetRef(oldHash)) &&
>+                  NS_SUCCEEDED(aNewURI->GetRef(newHash)) &&
>+                  !oldHash.Equals(newHash));

This looks wrong to me: it will bail out if aOldURI is "http://foo" and aNewURI is "http://foo#", no?  Please check that this either failed our existing tests or add a testcase...

Comment 28

[Dragana Damjanovic [:dragana]]

Attachment: bug_225910_v2.patch

Comment 29

[Dragana Damjanovic [:dragana]]

(In reply to Boris Zbarsky [:bz] from comment #27)
> 
> >+  NS_ENSURE_STATE(NS_SUCCEEDED(aOldURI->GetRef(oldHash)) &&
> >+                  NS_SUCCEEDED(aNewURI->GetRef(newHash)) &&
> >+                  !oldHash.Equals(newHash));
> 
> This looks wrong to me: it will bail out if aOldURI is "http://foo" and
> aNewURI is "http://foo#", no?  Please check that this either failed our
> existing tests or add a testcase...

There is a test for this (sorry I wanted to run it on try but try was closed on that day for a long time)

Comment 30

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=40a677845303&selectedJob=18909507

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/170e0ac6b292

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/170e0ac6b292