Remove hand-parsing for refs in content sinks and docshell

RESOLVED FIXED in Firefox 48

Status

()

Core
Networking
P5
normal
RESOLVED FIXED
15 years ago
2 years ago

People

(Reporter: bz, Assigned: dragana)

Tracking

(Blocks: 1 bug)

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [necko-backlog], URL)

Attachments

(1 attachment, 3 obsolete attachments)

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?
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...
Priority: -- → P1
Summary: Remove hand-parsing for refs in content sinks and docshell → [FIX]Remove hand-parsing for refs in content sinks and docshell
Target Milestone: --- → mozilla1.7alpha
Created attachment 135700 [details] [diff] [review]
Something like this

This should also resolve any case-sensitivity issues we had there...
Comment on attachment 135700 [details] [diff] [review]
Something like this

darin, what do you think?
Attachment #135700 - Flags: review?(darin)

Updated

15 years ago
Priority: P1 → --
Target Milestone: mozilla1.7alpha → ---

Comment 4

15 years ago
Oops ... sorry. I didnt intend to clobber the priority and milestone.
Priority: -- → P1
Target Milestone: --- → mozilla1.7alpha
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.
Er, ignore comment 5.  Wrong bug.  ;)

Comment 7

15 years ago
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?

Updated

15 years ago
Attachment #135700 - Flags: review?(darin) → review+
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

15 years ago
no, unfortunately wyciwyg just uses nsSimpleURI.
I realize that. The question is whether it should.  ;)

Comment 11

15 years ago
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.
> 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... :(
Perhaps we should just wontfix this, btw, with nice comments explaining why
people shouldn't bother....
Note that the "something like this" patch breaks <a href=#"> -- it treats it as
not having a ref at all.
Summary: [FIX]Remove hand-parsing for refs in content sinks and docshell → Remove hand-parsing for refs in content sinks and docshell
Priority: P1 → P5
Target Milestone: mozilla1.7alpha → ---
(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 #)
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.
(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)
Whiteboard: [necko-backlog]
(Assignee)

Comment 18

2 years ago
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?
Flags: needinfo?(bzbarsky)
See comment 16 for the issue with GetRef here.  Does GetHasRef let us distinguish between "http://foo" and "http://foo#"?
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 23

2 years ago
(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
(Assignee)

Comment 24

2 years ago
Created attachment 8735117 [details] [diff] [review]
bug_225910_v1.patch
Assignee: bzbarsky → dd.mozilla
Attachment #135700 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8735117 - Flags: review?(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.
Attachment #8735117 - Flags: review?(bzbarsky)
(Assignee)

Comment 26

2 years ago
Created attachment 8736214 [details] [diff] [review]
bug_225910_v2.patch
Attachment #8736214 - Flags: review?(bzbarsky)
(Assignee)

Updated

2 years ago
Attachment #8735117 - Attachment is obsolete: true
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...
Attachment #8736214 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 28

2 years ago
Created attachment 8737145 [details] [diff] [review]
bug_225910_v2.patch
Attachment #8736214 - Attachment is obsolete: true
Attachment #8737145 - Flags: review+
(Assignee)

Comment 29

2 years ago
(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)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 32

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/170e0ac6b292
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.