659698 - nsJSURI not correctly updated for the ref changes

Status: VERIFIED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

In particular, it at least needs equalsExceptRef and cloneIgnoringRef.

Are there other URI classes we missed?

Comment 1

[Daniel Holbert [:dholbert]]

oops!  (Why doesn't it have a compile error, then, for lacking impls for those methods?)

> Are there other URI classes we missed?

I don't think so, at least not top-level ones.  I searching for "public nsIURI"[1] and ": nsIURI"[2], and everything that turned up besides nsJSURI_base was covered in bug 308590.

It's possible that we missed some non-top-level URI classes that need customizations of those methods, though.  (In most cases I'd hope that the inherited versions would suffice, but good to be sure.)  I'll look more thoroughly for anything like that after fixing up nsJSURI.

[1]  http://mxr.mozilla.org/mozilla-central/search?string=public%20nsIURI
[2]  http://mxr.mozilla.org/mozilla-central/search?string=%3A%20nsIURI

Comment 2

[Boris Zbarsky [:bzbarsky]]

> Why doesn't it have a compile error, then

Because it inherits from a class that implements all of nsIURI by forwarding to an nsSimpleURI instance.  It then overrides some of the methods.

That's about to change on cedar to nsJSURI just inheriting from nsSimpleURI and overriding some methods, but the basic setup is the same either way.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to comment #2)
> > Why doesn't it have a compile error, then
> 
> Because it inherits from a class that implements all of nsIURI by forwarding
> to an nsSimpleURI instance.

Ah, "NS_FORWARD_NSIURI".  I see.

Comment 4

[Daniel Holbert [:dholbert]]

That's the only instance of NS_FORWARD_NSIUR* in our codebase, happily.

http://mxr.mozilla.org/mozilla-central/search?string=NS_FORWARD_NSIUR

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to comment #2)
> That's about to change on cedar to nsJSURI just inheriting from nsSimpleURI

(in Bug 647570, for reference)

Marking this bug as depending on that bug, since I'm basing the patch here on top of that.

Comment 6

[Masatoshi Kimura [:emk]]

Does this change "break" javascript: URIs such as |javascript:alert('#')|?
Yeah, those URIs should be parsed as |javascript:alert('| + |#')| per URI spec, but it will have a much more impact on Web compat than data: URIs.

Comment 7

[Daniel Holbert [:dholbert]]

Yes, I believe bug 308590 "breaks" such URIs. (unrelated to this bug here, though)

I think you should be able to fix that by replacing "#" with "%23" in such URIs.  (At least, that works fine in data URIs.)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Here's a patch, with an xpcshell test included. It layers on top of Bug 647570.

(In reply to comment #0)
> In particular, it at least needs equalsExceptRef and cloneIgnoringRef.

I don't think we actually need a specialized cloneIgnoringRef after Bug 647570, do we?  The only custom data we have in nsJSURI is mBaseURI, and that already gets cloned in the StartClone() impl. (I added a comment in that method, clarifying its ref-handling behavior.)

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: fix v1a

(Fixed a few small things I noticed when glancing over the first attachment.)

Comment 10

[Daniel Holbert [:dholbert]]

I've also changed the logic & up-front-expectations in nsJSURI::EqualsInternal to more closely match nsSimpleURI::EqualsInternal, since there's no real reason they should differ on those.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> I don't think we actually need a specialized cloneIgnoringRef after Bug 647570, > do we? 

We might not, if StartClone does the right thing.

I do worry a bit about the compat issue that emk brings up.  What do other browsers do with javascript: URIs containing '#'?  Do we need to just have the JS protocol handler's newURI escape '#' in such URIs before passing it off to nsSimpleURI?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 535247 [details] [diff] [review]
fix v1a

r=me.  Thanks!

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: demo testcase for javascript URIs that contain "#"

(In reply to comment #11)
> I do worry a bit about the compat issue that emk brings up.

Sorry -- it looks like I was wrong about that, actually.  '#' still "works" in javascript: URIs after all, both before & after this bug's patch is applied.

Specifically, if I put:
> javascript:var foo='#'; new Date()
into my URLbar, then the date is correctly displayed in the urlbar.  And the first line in the attached testcase successfully alerts a string that contains a '#' character.  (Opera & chromium match our behavior on this testcase, fwiw)

This probably still works because the script text is extracted from the URL via nsJSURI::GetSpec or ::GetPath (which still include the ref, and hence behave the same before & after bug 308590)

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to comment #13)
> Specifically, if I put:
> > javascript:var foo='#'; new Date()
> into my URLbar, then the date is correctly displayed in the urlbar.

er, I meant "displayed in the page body".

Comment 15

[Boris Zbarsky [:bzbarsky]]

Ah, yes.  GetPath() is used to get the string to evaluate.  OK, good.

Comment 16

[Daniel Holbert [:dholbert]]

Landed patch on cedar:
http://hg.mozilla.org/projects/cedar/rev/01a96f067572

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to comment #1)
> It's possible that we missed some non-top-level URI classes that need
> customizations of those methods, though.  I'll look more
> thoroughly for anything like that after fixing up nsJSURI.

Just to follow up on this - I spent some time looking through mxr (searching ns.*URI, ns.*URL, 'public nsIURI' / ': nsIURI', public nsSimpleURI, public nsStandardURL, etc)  and didn't find any other classes that I'd missed in bug 308590.

(As noted in comment 2-4, I only missed this one because it forwarded its methods to a nsSimpleURI data member via NS_FORWARD_NSIURI, and it's the only class in our codebase to implement nsIURI or nsIURL in that way, so there shouldn't be any other issues like this hiding anywhere.)

Comment 18

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/01a96f067572

Comment 19

[Asa Dotzler [:asa]]

please nominate the patch for approval Aurora for 6. You have about 5 weeks to get it approved and landed there.

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 535247 [details] [diff] [review]
fix v1a

Requesting approval to land on aurora.

This patch changes a method signature in one nsIURI implementation that I'd missed in bug 308590.  I'd missed it because it blanket-forwarded all not-explicitly-overridden nsIURI methods to another object via the NS_FORWARD_NSIURI macro.  I've verified that no other classes used that macro.

Reward: Fixes regression in nsJSURI::Equals behavior. Includes testcase.
Risk: Low.

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 535247 [details] [diff] [review]
fix v1a

Canceling approval request - as noted on bug 647570, I'm going to write an aurora-specific version of this that doesn't layer on top of bug 647570.

Comment 22

[Daniel Holbert [:dholbert]]

Attachment: fix for aurora

Here's the fix for aurora.

As bug 308590 did for other nsIURI classes, this patch just shifts the existing Equals() & Clone() functionality into "Internal" methods, and then uses one-liners for the normal & ref-ignoring versions of those functions.

Comment 23

[Daniel Holbert [:dholbert]]

This includes the xpcshell test from the trunk patch.

I verified that the test fails on current aurora, but passes with the fix applied.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 536744 [details] [diff] [review]
fix for aurora

r=me

Comment 25

[Daniel Holbert [:dholbert]]

Landed on aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/84ef9983832e

Comment 26

[Trif Andrei-Alin[:AlinT]]

Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0

I have runned the tescase on these builds and it works fine.
But i encountered a problem on Mac: clicking the alerts and hitting the Esc button makes my mouse disappear.
Thanks!