409885 - SetDomain shouldn't do string manipulation of URIs

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Whenever I see this sort of thing I get a little weirded out.  And one of the results of the way it works now is that on non-hierarchical URIs we do very very weird things (instead of just throwing, which is what we _should_ be doing, imo).

I left the SetUserPass() in there because that's what we do now, but I'm not really sure we should.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Reopening.  Looks like this method doesn't work.  :(  Serves me right for assuming things...

Filed bug 410293 on making it work.

Comment 3

[Mike Schroepfer]

Comment on attachment 294587 [details] [diff] [review]
Fix

Clearing approval since this patch was backed out and we have closed for 1.9.

Comment 4

[Jeff Walden [:Waldo]]

HTML5 currently says document.domain represents only the hostname portion of the origin, not the port, and you can't set it to a hostport combination.  If that sticks (and as a complexity-reducing measure I'm in favor), we could reapply the patch with SetHostPort changed to SetHost and probably be good to go.

Comment 5

[Boris Zbarsky [:bzbarsky]]

If that spec survives real-world use cases, sure.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Does the spec survive real-world use cases?  Or should I fix bug 410293 so this one can get fixed?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Does the spec survive real-world use cases?

No idea.  Do other browsers implement it?

Comment 8

[Dragana Damjanovic [:dragana]]

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

Comment 9

[Dragana Damjanovic [:dragana]]

I was cleaning up some necko bugs and this one was referred by one of them. It is a easy fix.

Comment 10

[Dragana Damjanovic [:dragana]]

Attachment: bug_409885.patch

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8733991 [details] [diff] [review]
bug_409885.patch

r=me.  Thanks for following up on this!

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8733991 [details] [diff] [review]
bug_409885.patch

Er, actually just realized a problem in the new code.  This:

>+  if (NS_FAILED(rv2)) {
>+    rv.Throw(rv2);
>   }

should be returning after the Throw() call, no?  Same in the other two places that do this.  Please fix before pushing.

Comment 13

[Dragana Damjanovic [:dragana]]

(In reply to Boris Zbarsky [:bz] from comment #12)
> Comment on attachment 8733991 [details] [diff] [review]
> bug_409885.patch
> 
> Er, actually just realized a problem in the new code.  This:
> 
> >+  if (NS_FAILED(rv2)) {
> >+    rv.Throw(rv2);
> >   }
> 
> should be returning after the Throw() call, no?  Same in the other two
> places that do this.  Please fix before pushing.

yes, I will fix it.

Comment 14

[Dragana Damjanovic [:dragana]]

Attachment: bug_409885.patch

Comment 15

[Dragana Damjanovic [:dragana]]

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

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d81ec460382e

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d81ec460382e