1280584 - About protocol handler totally ignores baseURI and doesn't work for relative (#foo) URIs

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[:Gijs (he/him)]

So if you pass a valid about: URI as the base URI and e.g. "#" or "#somehash" or "#gohere" as a the main uri string, the result is that we do this:

http://searchfox.org/mozilla-central/rev/970569ad57ac4436ff31aa2ac63f38ed3ee2932d/netwerk/protocol/about/nsAboutProtocolHandler.cpp#101-117

    rv = url->SetSpec(aSpec);
    if (NS_FAILED(rv)) {
        return rv;
    }

Because it turns out that trying to set the spec to "#" gives you a 'malformed URI' error, well, that won't work.

This means that hash links on about: pages won't work, and even if we use <a href="#"> with a click handler, that link now won't be focusable, because the focusable determination checks if the href is actually valid, which it isn't in this case. This causes bug 1280472. We'll probably just work around it there, but it would be nice to fix this issue in the about protocol handler...

Comment 1

[Boris Zbarsky [:bzbarsky]]

You basically want something like http://hg.mozilla.org/mozilla-central/rev/dbce2f14576e I would think.  See also some relevant discussion in bug 1157953.

Sounds like we should just do this in the about protocol handler for now, since the "mutate it after" approach won't work for immutable URIs.  Or better yet, we could add a cloneWithNewRef thing...

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1280584 - implement cloneWithNewRef,

Review commit: https://reviewboard.mozilla.org/r/64936/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64936/

Comment 3

[:Gijs (he/him)]

This was... more work than I anticipated. Which makes me wonder if this is anywhere near right. :-)

Comment 4

[:Gijs (he/him)]

FWIW, I didn't remove the data-URI-specific dealings with relative URIs because I'm not sure if we should expect people to get and interact with that/those protocol handlers individually and continue to support people calling newURI on them "manually", or not. It also feels like cleanup we could do in another bug, esp. if it might affect tests...

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8771991 [details]
Bug 1280584 - implement cloneWithNewRef,

https://reviewboard.mozilla.org/r/64936/#review61940

::: modules/libjar/nsJARURI.cpp:539
(Diff revision 1)
> +
> +    nsCOMPtr<nsIJARURI> uri;
> +    rv = CloneWithJARFileInternal(mJARFile, eIgnoreRef, getter_AddRefs(uri));
> +    if (NS_FAILED(rv)) return rv;
> +
> +    uri->SetRef(newRef);

I'd prefer it if we added a ref argument to CloneWithJARFileInternal and passed eReplaceRef to it here.

::: netwerk/base/nsSimpleURI.cpp:514
(Diff revision 1)
>      url->mScheme = mScheme;
>      url->mPath = mPath;
>      if (refHandlingMode == eHonorRef) {
>          url->mRef = mRef;
>          url->mIsRefValid = mIsRefValid;
> +    } else if (refHandlingMode == eReplaceRef) {

Why is this needed, if we're passing the refHandlingMode and newRef to StartClone?  Shouldn't the StartClone implementation dea  If not, why are we changing StartClone at all?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Since $%^$%&%^&%^ mozreview apparently won't let me comment while changing the reviewer: I'm not a necko peer anymore; one of them needs to review this patch.

Comment 7

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #5)
> ::: netwerk/base/nsSimpleURI.cpp:514
> (Diff revision 1)
> >      url->mScheme = mScheme;
> >      url->mPath = mPath;
> >      if (refHandlingMode == eHonorRef) {
> >          url->mRef = mRef;
> >          url->mIsRefValid = mIsRefValid;
> > +    } else if (refHandlingMode == eReplaceRef) {
> 
> Why is this needed, if we're passing the refHandlingMode and newRef to
> StartClone?  Shouldn't the StartClone implementation dea  If not, why are we
> changing StartClone at all?

I changed startclone because it turned out that in the nsSimpleNestedURI case, we needed to do stuff in there. In particular, the inner URI wants its #ref updated, otherwise, these two URIs:

newURI("about:blank#", null, null)

newURI("about:blank").cloneWithNewRef("#")

had the same spec but failed .equals() checks because the former had the ref in the inner URI and the latter didn't.

Without looking at the patch again, I can look at pushing the SetRef() into StartClone in nsSimpleURI. I thought it'd be odd to do that because the eHonorRef case isn't in there...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Yeah, I wonder why the eHonorRef is not there.  Maybe because we wanted to share the code...

Anyway, the right answer might be to factor out the part of SetRef that comes after the mMutable check and use it here.  But that's something to check with the necko folks.  ;)

Comment 9

[Valentin Gosu [:valentin] (he/him)]

Even though I understand where this is coming from, I'm not really enthusiastic about adding another method to nsIURI, when we already have .clone() and .ref, which should accomplish the same thing.

From what I can tell, the problem here is that nsSimpleNestedURI::StartClone also sets .mutable = false, right?
If there's really no other way to work around that, I guess we could add cloneWithNewRef. The patch looks fine, btw :)

Note: there is and xpcshell-test that is failing on try, also a WPT with an UNEXPECTED-PASS :)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Note that I strongly believe the right long-term goal should be to make all URIs always immutable.  So the right long-term solution is either CloneWithNewRef or making SetRef return a new URI.

Comment 11

[:Gijs (he/him)]

(In reply to Valentin Gosu [:valentin] from comment #9)
> Even though I understand where this is coming from, I'm not really
> enthusiastic about adding another method to nsIURI, when we already have
> .clone() and .ref, which should accomplish the same thing.
> 
> From what I can tell, the problem here is that nsSimpleNestedURI::StartClone
> also sets .mutable = false, right?
> If there's really no other way to work around that, I guess we could add
> cloneWithNewRef. The patch looks fine, btw :)

Well, StartClone isn't an xpcom method, and not implemented on every nsIURI. If we want a generic way for the IO service to say newURI("#foo", <whatever>, someURI) should be interpreted as just a clone of someURI with a different ref, it would need a way to get a temporarily-mutable URI irrespective of the implementing class of someURI, to be able to set 'ref' on it. That seems anywhere between tricky and impossible, and therefore not very attractive... would you agree? It also goes against bz's idea that we should ideally eradicate mutable URIs at some point, if we allow external consumers to get references to mutable copies (even if right now the only consumer of such a hypothetical API would be the IO service which would never hand out the refs).

So I think CloneWithNewRef might be our best bet. Does that reasoning make sense or do you think I'm missing something (very possible!) ?

> Note: there is and xpcshell-test that is failing on try, also a WPT with an
> UNEXPECTED-PASS :)

Yeah, I saw the xpcshell-test... I will need to look at that and the WPT pass in more detail as I adopt the patch to feedback. Would like to be sure about the direction before I do that though. :-)

Comment 12

[Valentin Gosu [:valentin] (he/him)]

(In reply to :Gijs Kruitbosch from comment #11)
> So I think CloneWithNewRef might be our best bet. Does that reasoning make
> sense or do you think I'm missing something (very possible!) ?
> 

I think you're correct. I keep looking at it, and don't see any straightforward way of doing this.
I think the bug :bz is referring to is bug 922464. It's been a while since I last looked at it, but maybe it's time we took care of it.

> > Note: there is and xpcshell-test that is failing on try, also a WPT with an
> > UNEXPECTED-PASS :)
> 
> Yeah, I saw the xpcshell-test... I will need to look at that and the WPT
> pass in more detail as I adopt the patch to feedback. Would like to be sure
> about the direction before I do that though. :-)

I usually regard unexpected-pass as a good thing, but for this it would be interesting to know _why_ it happens :)

Comment 13

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8771991 [details]
Bug 1280584 - implement cloneWithNewRef,

https://reviewboard.mozilla.org/r/64936/#review62244

Looks good.

Comment 14

[:Gijs (he/him)]

Comment on attachment 8771991 [details]
Bug 1280584 - implement cloneWithNewRef,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64936/diff/1-2/

Comment 15

[:Gijs (he/him)]

The new version addresses the JAR URI concerns and moves the handling of the ref into a mini-function that gets called from startclone and overridden copies of startclone in subclasses. It also fixes the xpcshell failure, which is because the sanitizer uses about:blank as a base URI, and it didn't use to be possible to create a URI for <a href='#'>Link</a> with the sanitizer on that document root, but it now is. That seems correct, so I simply changed the expected outcomes of the sanitizer there.

Finally, I think the web unexpected pass stuff is just a high-frequency intermittent: bug 1211316. A number of the items starred there are also unexpected-pass (rather than timed out, which is what the bug is filed for).

Comment 16

[:Gijs (he/him)]

Comment on attachment 8771991 [details]
Bug 1280584 - implement cloneWithNewRef,

Re-requesting review to make sure this change is OK...

Comment 17

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8771991 [details]
Bug 1280584 - implement cloneWithNewRef,

https://reviewboard.mozilla.org/r/64936/#review63716

The changes look fine, with this minor nit.

::: dom/jsurl/nsJSProtocolHandler.cpp:1370
(Diff revisions 1 - 2)
> -nsJSURI::StartClone(mozilla::net::nsSimpleURI::RefHandlingEnum /* ignored */,
> -                    const nsACString& /* ignored */)
> +nsJSURI::StartClone(mozilla::net::nsSimpleURI::RefHandlingEnum refHandlingMode,
> +                    const nsACString& newRef)
>  {
>      nsCOMPtr<nsIURI> baseClone;
>      if (mBaseURI) {
>        // Note: We preserve ref on *base* URI, regardless of ref handling mode.

This comment is no longer valid.

Comment 18

[:Gijs (he/him)]

(In reply to Valentin Gosu [:valentin] from comment #17)
> Comment on attachment 8771991 [details]
> Bug 1280584 - implement cloneWithNewRef,
> 
> https://reviewboard.mozilla.org/r/64936/#review63716
> 
> The changes look fine, with this minor nit.
> 
> ::: dom/jsurl/nsJSProtocolHandler.cpp:1370
> (Diff revisions 1 - 2)
> > -nsJSURI::StartClone(mozilla::net::nsSimpleURI::RefHandlingEnum /* ignored */,
> > -                    const nsACString& /* ignored */)
> > +nsJSURI::StartClone(mozilla::net::nsSimpleURI::RefHandlingEnum refHandlingMode,
> > +                    const nsACString& newRef)
> >  {
> >      nsCOMPtr<nsIURI> baseClone;
> >      if (mBaseURI) {
> >        // Note: We preserve ref on *base* URI, regardless of ref handling mode.
> 
> This comment is no longer valid.

I think it is? mBaseURI for the new URI is the same as mBaseURI for the old URI. We only change/keep/remove the ref on the clone of the URI itself (rather than on mBaseURI). That behaviour is the same after the patch, and matches the behaviour in Equals/EqualsInternal.

This also makes sense, so that if you have an mBaseURI of "http://www.mozilla.org/#foo" for your javascript: URI, changing the ref on the js URI shouldn't change the "#foo" on the base URI.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8f1980fa5bee
implement cloneWithNewRef, r=valentin

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out because it apparently broke artifact builds like https://treeherder.mozilla.org/logviewer.html#?job_id=981339&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/8c361c7cc683

Comment 21

[:Gijs (he/him)]

(In reply to Wes Kocher (:KWierso) from comment #20)
> I had to back this out because it apparently broke artifact builds like
> https://treeherder.mozilla.org/logviewer.html#?job_id=981339&repo=autoland
> 
> https://hg.mozilla.org/integration/autoland/rev/8c361c7cc683

There are a few things wrong here... In no particular order:

- the startup cache precompilation works fine on my machine with that cset built locally (and clearly works for the "normal" builds) so something fishy is going on;
- but the same failure state happens on try - 'real' builds work, artifact ones fail (manually added the [AB] build);
- artifact builds are tier 2, so AIUI I shouldn't have been backed out over them;
- artifact builds on infra should really be using their own pushhead for artifacts. And I'm sure they would on inbound/fx-team/b2g-inbound/m-c, because those are the trees that it looks for pushheads in. Autoland is not one of those trees, so AIUI any material binary/interface change that lands on autoland is going to cause issues with those builds, because it grabs artifacts from some other unrelated build and then the idl/xpt files no longer match either the frontend code or the binary code (I forget if artifact builds do their own idl compilation or not) and so life gets sucky. This is borne out by the log which says:

12:53:08     INFO -  Installing from remote pushhead ff1ef8ec0fd800bf6856c1572c3b1610c45e9b6a on mozilla-central

which, um, yes, isn't going to work because I changed nsIURI, and so things get very confused.

AIUI this means I can safely reland on fx-team or inbound, so I'll go and do that...

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/f4ed38f18dbc
implement cloneWithNewRef and thereby make hash/ref links use a simple unified codepath in the IO service, r=valentin

Comment 23

[:Gijs (he/him)]

filed bug 1289695 for the artifact build vs. autoland malarkey.

Comment 24

[Carsten Book [:Tomcat]]

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

Comment 25

[Jorg K (CEST = GMT+2)]

This has busted Thunderbird builds:

Windows:
http://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-win64/1469649266/comm-central-win64-bm91-build1-build39.txt.gz

nsLDAPURL.obj : error LNK2001: unresolved external symbol "public: virtual enum nsresult __cdecl nsLDAPURL::CloneWithNewRef(class nsACString_internal const &,class nsIURI * *)" (?CloneWithNewRef@nsLDAPURL@@UEAA?AW4nsresult@@AEBVnsACString_internal@@PEAPEAVnsIURI@@@Z)

nsAddbookUrl.obj : error LNK2001: unresolved external symbol "public: virtual enum nsresult __cdecl nsAddbookUrl::CloneWithNewRef(class nsACString_internal const &,class nsIURI * *)" (?CloneWithNewRef@nsAddbookUrl@@UEAA?AW4nsresult@@AEBVnsACString_internal@@PEAPEAVnsIURI@@@Z)

nsMailboxUrl.obj : error LNK2001: unresolved external symbol "public: virtual enum nsresult __cdecl nsMsgMailNewsUrl::CloneWithNewRef(class nsACString_internal const &,class nsIURI * *)" (?CloneWithNewRef@nsMsgMailNewsUrl@@UEAA?AW4nsresult@@AEBVnsACString_internal@@PEAPEAVnsIURI@@@Z)

nsPop3URL.obj : error LNK2001: unresolved external symbol "public: virtual enum nsresult __cdecl nsMsgMailNewsUrl::CloneWithNewRef(class nsACString_internal const &,class nsIURI * *)" (?CloneWithNewRef@nsMsgMailNewsUrl@@UEAA?AW4nsresult@@AEBVnsACString_internal@@PEAPEAVnsIURI@@@Z)

nsNntpUrl.obj : error LNK2001: unresolved external symbol "public: virtual enum nsresult __cdecl nsMsgMailNewsUrl::CloneWithNewRef(class nsACString_internal const &,class nsIURI * *)" (?CloneWithNewRef@nsMsgMailNewsUrl@@UEAA?AW4nsresult@@AEBVnsACString_internal@@PEAPEAVnsIURI@@@Z)

nsMsgMailNewsUrl.obj : error LNK2001: unresolved external symbol "public: virtual enum nsresult __cdecl nsMsgMailNewsUrl::CloneWithNewRef(class nsACString_internal const &,class nsIURI * *)" (?CloneWithNewRef@nsMsgMailNewsUrl@@UEAA?AW4nsresult@@AEBVnsACString_internal@@PEAPEAVnsIURI@@@Z)

nsSmtpUrl.obj : error LNK2001: unresolved external symbol "public: virtual enum nsresult __cdecl nsMsgMailNewsUrl::CloneWithNewRef(class nsACString_internal const &,class nsIURI * *)" (?CloneWithNewRef@nsMsgMailNewsUrl@@UEAA?AW4nsresult@@AEBVnsACString_internal@@PEAPEAVnsIURI@@@Z)

nsImapUrl.obj : error LNK2001: unresolved external symbol "public: virtual enum nsresult __cdecl nsMsgMailNewsUrl::CloneWithNewRef(class nsACString_internal const &,class nsIURI * *)" (?CloneWithNewRef@nsMsgMailNewsUrl@@UEAA?AW4nsresult@@AEBVnsACString_internal@@PEAPEAVnsIURI@@@Z)

JaUrl.obj : error LNK2001: unresolved external symbol "public: virtual enum nsresult __cdecl nsMsgMailNewsUrl::CloneWithNewRef(class nsACString_internal const &,class nsIURI * *)" (?CloneWithNewRef@nsMsgMailNewsUrl@@UEAA?AW4nsresult@@AEBVnsACString_internal@@PEAPEAVnsIURI@@@Z)

nsSmtpUrl.obj : error LNK2001: unresolved external symbol "public: virtual enum nsresult __cdecl nsMailtoUrl::CloneWithNewRef(class nsACString_internal const &,class nsIURI * *)" (?CloneWithNewRef@nsMailtoUrl@@UEAA?AW4nsresult@@AEBVnsACString_internal@@PEAPEAVnsIURI@@@Z)

xul.dll : fatal error LNK1120: 4 unresolved externals

Mac:
http://archive.mozilla.org/pub/thunderbird/try-builds/[e-mail]-85dbd3006eabe4696409494cdf0f742a774830ae/try-comm-central-macosx64/try-comm-central-macosx64-bm87-try1-build95.txt.gz

ld: warning: could not create compact unwind for _ffi_call_unix64: does not use RBP or RSP based frame
Undefined symbols for architecture x86_64:
  "__ZN11nsMailtoUrl15CloneWithNewRefERK19nsACString_internalPP6nsIURI", referenced from:
      __ZTV11nsMailtoUrl in nsSmtpUrl.o
  "__ZN9nsLDAPURL15CloneWithNewRefERK19nsACString_internalPP6nsIURI", referenced from:
      __ZTV9nsLDAPURL in nsLDAPURL.o
  "__ZThn8_N11nsMailtoUrl15CloneWithNewRefERK19nsACString_internalPP6nsIURI", referenced from:
      __ZTV11nsMailtoUrl in nsSmtpUrl.o
  "__ZN16nsMsgMailNewsUrl15CloneWithNewRefERK19nsACString_internalPP6nsIURI", referenced from:
      __ZTV16nsMsgMailNewsUrl in nsMsgMailNewsUrl.o
      __ZTV9nsSmtpUrl in nsSmtpUrl.o
      __ZTV9nsImapUrl in nsImapUrl.o
      __ZN7mozilla8mailnews17JaCppUrlDelegator5Super15CloneWithNewRefERK19nsACString_internalPP6nsIURI in JaUrl.o
      __ZTVN7mozilla8mailnews12JaBaseCppUrlE in JaUrl.o
      __ZTV12nsMailboxUrl in nsMailboxUrl.o
      __ZTV9nsPop3URL in nsPop3URL.o
      ...
  "__ZN12nsAddbookUrl15CloneWithNewRefERK19nsACString_internalPP6nsIURI", referenced from:
      __ZTV12nsAddbookUrl in nsAddbookUrl.o
ld: symbol(s) not found for architecture x86_64

Can you give us a hand. We're not calling the new function CloneWithNewRef() so I'm a bit surprised this is happening.

Comment 26

[Frank-Rainer Grahl (:frg)]

Seamonkey building is busted too because of this. No wonder its the same mailnews code.

Comment 27

[Jorg K (CEST = GMT+2)]

Funny thing is: It compiles, but it doesn't link and can't find the new function we don't even use. Gijs is going on PTO later today (in Europe), so perhaps Valentin can lend us a hand.

Comment 28

[Valentin Gosu [:valentin] (he/him)]

The definition of nsIURI has changed. You should add the following code:

For URLs that may have a ref:
NS_IMETHODIMP
nsMailtoUrl::CloneWithNewRef(const nsACString& newRef, nsIURI **_newURI)
{
  nsresult rv = Clone(_newURI);
  if (NS_FAILED(rv)) return rv;
  return (*_newURI)->SetRef(newRef);
}
For URLs that can't have a ref:
NS_IMETHODIMP
nsMailtoUrl::CloneWithNewRef(const nsACString& newRef, nsIURI **_newURI)
{
  return Clone(_newURI);
}

Comment 29

[Jorg K (CEST = GMT+2)]

Thanks!!

Comment 32

[omry.shap]

seems like there's a regression in with this bug in 59.0.1 (64bit) or possibly sooner, I didn't check.

see this example by @elm19087 from bug 841920:

http://www.snappymaria.com/misc/svg-id-test/

Comment 33

[:Gijs (he/him)]

(In reply to omry.shap from comment #32)
> seems like there's a regression in with this bug in 59.0.1 (64bit) or
> possibly sooner, I didn't check.
> 
> see this example by @elm19087 from bug 841920:
> 
> http://www.snappymaria.com/misc/svg-id-test/

This is being tracked in bug 1447262.