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

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [necko-next])

Attachments

(1 attachment)

Assignee

Description

3 years ago
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...
Assignee

Updated

3 years ago
See Also: → 1280888
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...
Whiteboard: [necko-next]
Assignee

Comment 3

3 years ago
This was... more work than I anticipated. Which makes me wonder if this is anywhere near right. :-)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Assignee

Comment 4

3 years ago
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
Attachment #8771991 - Flags: review?(bzbarsky) → review?(jduell.mcbugs)
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?
Attachment #8771991 - Flags: review?(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.
Attachment #8771991 - Flags: review?(bzbarsky)
Assignee

Comment 7

3 years ago
(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...
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.  ;)

Updated

3 years ago
Attachment #8771991 - Flags: review?(jduell.mcbugs) → review?(valentin.gosu)
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 :)
Flags: needinfo?(gijskruitbosch+bugs)
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.
Assignee

Comment 11

3 years ago
(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. :-)
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(valentin.gosu)
(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 :)
Flags: needinfo?(valentin.gosu)
Comment on attachment 8771991 [details]
Bug 1280584 - implement cloneWithNewRef,

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

Looks good.
Attachment #8771991 - Flags: review?(valentin.gosu) → review+
Assignee

Comment 14

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

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

Comment 15

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

Comment 16

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

Re-requesting review to make sure this change is OK...
Attachment #8771991 - Flags: review+ → review?(valentin.gosu)
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.
Attachment #8771991 - Flags: review?(valentin.gosu) → review+
Assignee

Comment 18

3 years ago
(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.
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
Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Comment 21

3 years ago
(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...
Flags: needinfo?(gijskruitbosch+bugs)

Comment 22

3 years ago
Pushed by gijskruitbosch@gmail.com:
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
Assignee

Comment 23

3 years ago
filed bug 1289695 for the artifact build vs. autoland malarkey.

Comment 24

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f4ed38f18dbc
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

Updated

3 years ago
Depends on: 1289872

Comment 25

3 years ago
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/makemyday@gmx-topmail.de-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.
Flags: needinfo?(gijskruitbosch+bugs)

Updated

3 years ago
No longer depends on: 1289872
Seamonkey building is busted too because of this. No wonder its the same mailnews code.

Comment 27

3 years ago
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.
Flags: needinfo?(valentin.gosu)

Updated

3 years ago
Blocks: 1289933
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);
}
Flags: needinfo?(valentin.gosu)

Comment 29

3 years ago
Thanks!!
Flags: needinfo?(gijskruitbosch+bugs)

Updated

3 years ago
Blocks: 1290541

Updated

3 years ago
Depends on: 1290927
Duplicate of this bug: 841920

Updated

3 years ago
Duplicate of this bug: 1312068

Comment 32

a year ago
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/
Assignee

Comment 33

a year ago
(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.
See Also: → 1447262
You need to log in before you can comment on or make changes to this bug.