Last Comment Bug 659698 - nsJSURI not correctly updated for the ref changes
: nsJSURI not correctly updated for the ref changes
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
Depends on: 647570
Blocks: 308590
  Show dependency treegraph
 
Reported: 2011-05-25 10:55 PDT by Boris Zbarsky [:bz] (Out June 25-July 6)
Modified: 2013-12-27 14:25 PST (History)
5 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
fixed
-
unaffected


Attachments
fix v1 (4.66 KB, patch)
2011-05-25 17:41 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Review
fix v1a (4.61 KB, patch)
2011-05-25 17:59 PDT, Daniel Holbert [:dholbert]
bzbarsky: review+
Details | Diff | Review
demo testcase for javascript URIs that contain "#" (216 bytes, text/html)
2011-05-25 22:34 PDT, Daniel Holbert [:dholbert]
no flags Details
fix for aurora (4.44 KB, patch)
2011-06-01 14:30 PDT, Daniel Holbert [:dholbert]
bzbarsky: review+
jst: approval‑mozilla‑aurora+
Details | Diff | Review

Description Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-25 10:55:18 PDT
In particular, it at least needs equalsExceptRef and cloneIgnoringRef.

Are there other URI classes we missed?
Comment 1 Daniel Holbert [:dholbert] 2011-05-25 12:01:18 PDT
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 [:bz] (Out June 25-July 6) 2011-05-25 12:04:40 PDT
> 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] 2011-05-25 12:17:08 PDT
(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] 2011-05-25 12:18:09 PDT
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] 2011-05-25 14:41:38 PDT
(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] 2011-05-25 15:53:24 PDT
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] 2011-05-25 15:57:28 PDT
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] 2011-05-25 17:41:54 PDT
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.)
Comment 9 Daniel Holbert [:dholbert] 2011-05-25 17:59:40 PDT
Created attachment 535247 [details] [diff] [review]
fix v1a

(Fixed a few small things I noticed when glancing over the first attachment.)
Comment 10 Daniel Holbert [:dholbert] 2011-05-25 18:01:38 PDT
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 [:bz] (Out June 25-July 6) 2011-05-25 18:29:11 PDT
> 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 [:bz] (Out June 25-July 6) 2011-05-25 22:09:21 PDT
Comment on attachment 535247 [details] [diff] [review]
fix v1a

r=me.  Thanks!
Comment 13 Daniel Holbert [:dholbert] 2011-05-25 22:34:20 PDT
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)
Comment 14 Daniel Holbert [:dholbert] 2011-05-25 22:36:17 PDT
(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 [:bz] (Out June 25-July 6) 2011-05-25 22:52:12 PDT
Ah, yes.  GetPath() is used to get the string to evaluate.  OK, good.
Comment 16 Daniel Holbert [:dholbert] 2011-05-26 01:22:49 PDT
Landed patch on cedar:
http://hg.mozilla.org/projects/cedar/rev/01a96f067572
Comment 17 Daniel Holbert [:dholbert] 2011-05-26 14:41:11 PDT
(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) 2011-05-27 01:05:42 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/01a96f067572
Comment 19 Asa Dotzler [:asa] 2011-05-31 14:47:33 PDT
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] 2011-05-31 15:30:29 PDT
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] 2011-06-01 12:03:09 PDT
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] 2011-06-01 14:30:37 PDT
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.
Comment 23 Daniel Holbert [:dholbert] 2011-06-01 14:31:48 PDT
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 [:bz] (Out June 25-July 6) 2011-06-01 20:47:51 PDT
Comment on attachment 536744 [details] [diff] [review]
fix for aurora

r=me
Comment 25 Daniel Holbert [:dholbert] 2011-06-03 10:58:12 PDT
Landed on aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/84ef9983832e
Comment 26 Trif Andrei-Alin[:AlinT] 2011-08-23 02:44:06 PDT
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!

Note You need to log in before you can comment on or make changes to this bug.