Note: There are a few cases of duplicates in user autocompletion which are being worked on.

nsJSURI not correctly updated for the ref changes

VERIFIED FIXED in Firefox 6

Status

()

Core
DOM
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: bz, Assigned: dholbert)

Tracking

({regression})

Trunk
mozilla7
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox6- fixed, firefox7- unaffected)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
In particular, it at least needs equalsExceptRef and cloneIgnoringRef.

Are there other URI classes we missed?
(Assignee)

Comment 1

6 years ago
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
(Reporter)

Comment 2

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

Comment 3

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

Comment 4

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

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

Comment 5

6 years ago
(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.
Depends on: 647570
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.
(Assignee)

Comment 7

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

Updated

6 years ago
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 8

6 years ago
Created attachment 535244 [details] [diff] [review]
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.)
(Assignee)

Updated

6 years ago
Attachment #535244 - Attachment is obsolete: true
(Assignee)

Comment 9

6 years ago
Created attachment 535247 [details] [diff] [review]
fix v1a

(Fixed a few small things I noticed when glancing over the first attachment.)
Attachment #535247 - Flags: review?(bzbarsky)
(Assignee)

Comment 10

6 years ago
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.
(Reporter)

Comment 11

6 years ago
> 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?
(Reporter)

Comment 12

6 years ago
Comment on attachment 535247 [details] [diff] [review]
fix v1a

r=me.  Thanks!
Attachment #535247 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 13

6 years ago
Created attachment 535270 [details]
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)
(Assignee)

Comment 14

6 years ago
(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".
(Reporter)

Comment 15

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

Comment 16

6 years ago
Landed patch on cedar:
http://hg.mozilla.org/projects/cedar/rev/01a96f067572
Flags: in-testsuite+
Whiteboard: fixed-in-cedar
(Assignee)

Comment 17

6 years ago
(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.)
Pushed:
http://hg.mozilla.org/mozilla-central/rev/01a96f067572
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla7

Comment 19

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

Comment 20

6 years ago
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.
Attachment #535247 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 21

6 years ago
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.
Attachment #535247 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 22

6 years ago
Created attachment 536744 [details] [diff] [review]
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.
Attachment #536744 - Flags: review?(bzbarsky)
Attachment #536744 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 23

6 years ago
This includes the xpcshell test from the trunk patch.

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

Comment 24

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

r=me
Attachment #536744 - Flags: review?(bzbarsky) → review+

Updated

6 years ago
Attachment #536744 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
status-firefox6: --- → affected
status-firefox7: --- → unaffected
tracking-firefox6: ? → -
tracking-firefox7: ? → -
(Assignee)

Comment 25

6 years ago
Landed on aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/84ef9983832e
status-firefox6: affected → fixed
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!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.