982428 - Interpret urls with bogus protocols ('site:mozilla.org firefox', 'define:serendipity') as keyword search queries rather than showing an error page

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Valentin Tsatskin [:vt]]

The rationale here is that users will probably not have the URI scheme model when interacting with our awesome bar. Thus showing an "invalid address" or "malformed URI" error will cause confusion and frustration when their intentions were to search the web.

Instead of showing the either error page, we should just direct them to an internet search.

Comment 1

[Jennifer Morrow [:Boriss] (UX)]

Note: this is particularly useful for Google-related commands such as mathematical equations, define:word, etc.  Right now, these work as inputs in Chrome's URL bar but not ours.

Comment 5

[Jennifer Morrow [:Boriss] (UX)]

I've added bug 982428 as a dup of this bug, because parsing non-URLs as searches should translate to utilizing Google-specific queries, such as "define:cat" and "site:mozilla.org free" if Google is the user's search provider.

Comment 6

[:Gijs (he/him)]

Marco, can you add this to the current iteration?

Comment 7

[Marco Mucci [:MarcoM]]

Added to Iteration 33.1

Comment 8

[:Gijs (he/him)]

(In reply to Valentin Tsatskin [:vt] from comment #0)
> The rationale here is that users will probably not have the URI scheme model
> when interacting with our awesome bar. Thus showing an "invalid address" or
> "malformed URI" error will cause confusion and frustration when their
> intentions were to search the web.
> 
> Instead of showing the either error page, we should just direct them to an
> internet search.

Just so we're on the same page, if I use "site:foo", I get neither of these errors, I get "The address wasn't understood" which isn't nearly as unfriendly as what you quoted.

What input are you using that triggers "invalid address" and/or "malformed URI"?

Comment 9

[:Gijs (he/him)]

Attachment: fix up 'valid' URIs that we can't handle, f?bz

So here's what I'm currently looking at... Boris, does this look non-crazy to you? The IsFrame() check in nsDocShell.h is basically a hacky version of 'is this a toplevel load' - not 100% sure if there's a better way to check for that. One thing I noticed in my testing is that this doesn't seem to work well with session restore: I still get a network error page with 'site:foo' instead of the google search page. Maybe that's something about my testing environment, or unrelated session store breakage, but it'd be good to sort that out...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8442528 [details] [diff] [review]
fix up 'valid' URIs that we can't handle, f?bz

>+        aFixupFlags & FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP) {

Parens around the bitwise and to be clear about precedence, please.

>+            if (!handlerExists) {
>+                CreateSearch(uriString, aPostData, aURI);

This is leaking the URI we created so far.  You want NS_RELEASE(*aURI) here before doing the CreateSearch thing.

Do we want to just key this new behavior off FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP, or add a new flag?  I expect the only user of FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP in practice is the URL bar?

>+    return CreateSearch(aKeyword, aPostData, aURI);

Why aKeyword, not keyword?

>+                responseType.Assign(mozKeywordSearch.get());

Drop the .get() bit.

>+                responseType.AssignLiteral("");

No need: it's already empty.

The docshell change I'm not convinced about.  I can see doing third-party fixup for a load from the URL bar, but if a web page does a location set on another web page, I don't think we want to be doing third-party fixup...

Comment 11

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #10)
> Comment on attachment 8442528 [details] [diff] [review]
> fix up 'valid' URIs that we can't handle, f?bz
> 
> >+        aFixupFlags & FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP) {
> 
> Parens around the bitwise and to be clear about precedence, please.
> 
> >+            if (!handlerExists) {
> >+                CreateSearch(uriString, aPostData, aURI);
> 
> This is leaking the URI we created so far.  You want NS_RELEASE(*aURI) here
> before doing the CreateSearch thing.

Interesting. I was under the impression that nsDefaultURIFixup does a lot of this type of overwriting... Now that I'm more awake, I'll have another look.

> Do we want to just key this new behavior off
> FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP, or add a new flag?  I expect the only user
> of FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP in practice is the URL bar?

This is hard to check because it gets passed in as a load flag. Here's users I found beside the obvious browser.js / utilityOverlay.js ones:

http://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp?mark=834,836#804

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#558

http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1442

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/ContentRestore.jsm#201

> 
> >+    return CreateSearch(aKeyword, aPostData, aURI);
> 
> Why aKeyword, not keyword?

Oversight, thanks for catchiing this. :-)

> The docshell change I'm not convinced about.  I can see doing third-party
> fixup for a load from the URL bar, but if a web page does a location set on
> another web page, I don't think we want to be doing third-party fixup...

This makes sense. I assumed that this flag wouldn't be passed in the cases you describe anyway... Furthermore, if you pass something that is parsable as a URL, the most we'll do is strip \r\n and some other bogus unescaped stuff out, mess with view-source and/or half-typed schemes / file links / backslashes, and then we actually return early because the block at http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDefaultURIFixup.cpp#257 will create a URL and the one at http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDefaultURIFixup.cpp#266 will return with that.

As it is, I'm not sure how best to distinguish these two cases - we could add an extra flag if we think that'd be enough of a distinction. Also not super sure why this code is even resetting the existing flag - why is it even possible we end up in that case? Bonsai (why, hello!) points to bug 263213, from which this isn't clear to me - that is, seems like something that was necessary then but shouldn't be necessary right now.

Then again, depending on cautious behaviour of nsDefaultURIFixup from the nsDocShell side seems like a bad move as well...

Comment 12

[:Gijs (he/him)]

Also, are there tests for this code? A casual search did not reveal any, which is mildly worrying... I will try and add one for this specific case, at least.

Comment 13

[:Gijs (he/him)]

Attachment: part 1: allow fixing up URIs with schemes that we can't handle,

I decided to add a new flag. I've also removed the INTERNAL equivalent to the fixup_scheme_typos flag because it was unused. This works, except that session restore still thinks that the url in use is the original one. This doesn't happen in the regular keyword search (e.g. just typing 'foo bar' in the location bar) so there must be something wrong with my patch, but I'm not sure what it is. :-( .

Comment 14

[:Gijs (he/him)]

Attachment: part 2: pass flag to do searches for unknown protocols,

This patch uses the flag everywhere (including mobile and metro...).

Comment 15

[Boris Zbarsky [:bzbarsky]]

> I was under the impression that nsDefaultURIFixup does a lot of this type of
> overwriting...

If you look carefully, you'll see that once *aURI is set, fixup either returns immediately or works with *aURI, not aURI.

> Here's users I found beside the obvious browser.js / utilityOverlay.js ones:

Hmm.  The frameloader owner is the only slightly fishy case, but that's coming from an attribute on the mozbrowser, not from web content, so should be ok.

The other uses look ok.

> I assumed that this flag wouldn't be passed in the cases you describe anyway...

Oh, I see.  This is removing the flag, not adding it.  And this is in the string-accepting function, which is never invoked by web content.  OK, that makes some sense, I think.  

On the other hand, note that the whole point of bug 263213 was that http://some-invalid-host-name typed in the URL bar should NOT do a search for "some-invalid-host-name".  That is, we want to treat the user typing "some-invalid-host-name" and "http://some-invalid-host-name" in the URL bar differently.  It's not clear to me whether this bug is trying to change that behavior; I think it's pretty clear that if a user types "http://some-invalid-host-name" they are not in fact trying to search the web.

> Also, are there tests for this code?

Probably not.  It doesn't help that it has multiple knobs that are set inside and outside of Gecko so it's not even clear where the tests should live and what they should test...  Adding some tests would be pretty awesome.

Comment 16

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #15)
> > I assumed that this flag wouldn't be passed in the cases you describe anyway...
> 
> Oh, I see.  This is removing the flag, not adding it.  And this is in the
> string-accepting function, which is never invoked by web content.  OK, that
> makes some sense, I think.  

Right. The issue is that it's removing it, which meant my code wasn't getting hit if I reused the flag in question.

> On the other hand, note that the whole point of bug 263213 was that
> http://some-invalid-host-name typed in the URL bar should NOT do a search
> for "some-invalid-host-name".  That is, we want to treat the user typing
> "some-invalid-host-name" and "http://some-invalid-host-name" in the URL bar
> differently.  It's not clear to me whether this bug is trying to change that
> behavior; I think it's pretty clear that if a user types
> "http://some-invalid-host-name" they are not in fact trying to search the
> web.

I'm not trying to change that, although changing a related case (typing "some-invalid-hostname" without a protocol prefix) is bug 693808's job. The current change I'm introducing (if I wrote it correctly) is very specific: it needs to be an external protocol for which we don't believe there's a handler (which basically guarantees that creating a channel will fail later on in the docshell code, causing the error page to show).

I went with the additional flag in the end anyway, as it seems safer considering there are so few tests. That said, now we're sending very similar flags in very many cases, which makes me wonder if I should be using the existing SCHEME_TYPOS flag... even if this isn't really a scheme typo. Maybe passing that flag could imply the new flag either way? That goes back to scary territory, in a certain sense - in another, I'm updating all the callers right now, which is also silly.

Needinfo for this: in your capacity as module peer/owner for docshell, can you recommend a course of action here wrt new flags / reusing flags / changing the impact of existing flags / renaming flags? (the impact of the flag addition is visible in the latest patches) :-)

> > Also, are there tests for this code?
> 
> Probably not.  It doesn't help that it has multiple knobs that are set
> inside and outside of Gecko so it's not even clear where the tests should
> live and what they should test...  Adding some tests would be pretty awesome.

I actually found a unit test for the urifixup case after asking this. I'll update that, but I'm considering adding a browser test for the actual functionality testing as well.

I found out the session restore issue is orthogonal, see bug 1027637.

Comment 17

[:Gijs (he/him)]

Attachment: part 1: allow fixing up URIs with schemes that we can't handle,

I realized that really, splitting off the CreateSearch bit was just breaking e10s. D'oh. Calling a search 'keywordtouri' is kind of confusing, but alright. :-)

Comment 18

[Boris Zbarsky [:bzbarsky]]

> which meant my code wasn't getting hit if I reused the flag in question.

Ah, because we do manage to produce a valid URI, I see.

I think making the scheme typos flag imply fixup for schemes we can't handle at all is pretty reasonable, actually.  It's a recent addition, and I can't think of cases offhand where we'd want to convert "ttp:" to "http:" but not convert unknown schemes into a search.

Comment 19

[:Gijs (he/him)]

Attachment: allow fixing up URIs with schemes that we can't handle,

I think this is ready for review. Try: https://tbpl.mozilla.org/?tree=Try&rev=82baf7c6d977

Comment 20

[Valentin Tsatskin [:vt]]

(In reply to :Gijs Kruitbosch from comment #8)
> (In reply to Valentin Tsatskin [:vt] from comment #0)
> > The rationale here is that users will probably not have the URI scheme model
> > when interacting with our awesome bar. Thus showing an "invalid address" or
> > "malformed URI" error will cause confusion and frustration when their
> > intentions were to search the web.
> > 
> > Instead of showing the either error page, we should just direct them to an
> > internet search.
> 
> Just so we're on the same page, if I use "site:foo", I get neither of these
> errors, I get "The address wasn't understood" which isn't nearly as
> unfriendly as what you quoted.
> 
> What input are you using that triggers "invalid address" and/or "malformed
> URI"?

I used the wrong name for "The address wasn't understood" (unknownProtocolFound)[1]; when I said "invalid address" I meant unknownProtocolFound.

As for malformedURI [2], I'm having trouble finding the example I had before which would trigger it that could be a search term. So I think just the unknownProtocolFound changes are good enough.

[1]:http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/overrides/appstrings.properties#8
[2]:http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/overrides/appstrings.properties#5

Comment 21

[:Gijs (he/him)]

Comment on attachment 8442951 [details] [diff] [review]
allow fixing up URIs with schemes that we can't handle,

Review of attachment 8442951 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsDefaultURIFixup.cpp
@@ +434,5 @@
> +
> +            defaultEngine->GetSubmission(NS_ConvertUTF8toUTF16(keyword),
> +                                         responseType,
> +                                         NS_LITERAL_STRING("keyword"),
> +                                         getter_AddRefs(submission));

To be clear, this hunk is not necessary for this patch, but it was cleanup I did while I was there, and so I left it in the patch.

::: docshell/base/nsDocShell.cpp
@@ -1571,5 @@
>      if (aLoadFlags & LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP)
>          flags |= INTERNAL_LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
>  
> -    if (aLoadFlags & LOAD_FLAGS_FIXUP_SCHEME_TYPOS)
> -        flags |= INTERNAL_LOAD_FLAGS_FIXUP_SCHEME_TYPOS;

Ditto here + removing the INTERNAL_... flag from the idl

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8442951 [details] [diff] [review]
allow fixing up URIs with schemes that we can't handle,

r=me if you add a comment in the IDL about 0x80 being a free bit.

Comment 23

[:Gijs (he/him)]

Attachment: allow fixing up URIs with schemes that we can't handle,

The new xpcshell test went orange on android only because it has an existing default search provider when running xpcshell tests, while desktop does not (?). I've updated the test to deal with that (hopefully), and repushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ebca7ffc4a3c . This also addresses the nit from the review.

Comment 24

[:Gijs (he/him)]

Comment on attachment 8443061 [details] [diff] [review]
allow fixing up URIs with schemes that we can't handle,

Carrying over r+

Comment 25

[:Gijs (he/him)]

Updating summary per comment #20 and discussion on IRC.

Comment 26

[:Gijs (he/him)]

... and considering there's an automated test here, and we ended up doing non-scary work for this, marking qa- instead of qa+

Comment 27

[dagger.bugzilla]

Does this patch obey keyword.enabled? There's no explicit check for it like there is for FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP.

Comment 29

[:Gijs (he/him)]

Comment on attachment 8443061 [details] [diff] [review]
allow fixing up URIs with schemes that we can't handle,

Boriss, I'm not fixing e.g. "60/5" in this bug, so you may or may not want to undo that duplicate. As I tried to make clear when I needinfo'd vt about this (who then seemed to agree that the case I've focused on is the main issue), for bugs like this, from an engineering perspective, it would be really helpful if there were clear steps to reproduce. Clearly, not every error page should be converted to a web search ("Firefox can't find the server for www.google.com" would be a fun one, for instance). I'm not really sure where UX feels the line is, and there are definitely cases that I'm not aware of (e.g. the 60/5 which was only mentioned in the now-duped-bug).

This bug only fixes the "thisisnotarealprotocol:stuff" case. It would probably make sense to file separate bugs if there are other cases where you think we should always do a keyword search, and to give concrete STR and expected/actual results there.

(In reply to dagger.bugzilla from comment #27)
> Does this patch obey keyword.enabled? There's no explicit check for it like
> there is for FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP.

Ugh. I forgot we have prefs for every little thing we do. IMO, we should just remove keyword.enabled, it's a footgun type pref. Anyway, different debate.

I have a fixed version locally that does check the pref, but I'm more worried about how the test continues to fail on android right now, so I'm not putting up a new patch until I've figured that out.

Comment 30

[:Gijs (he/him)]

Attachment: allow fixing up URIs with schemes that we can't handle,

Now with pref check. Test disabled on Android because of bug 1028091. Android seems to already have a prompt implemented for cases like this, with a Search button next to it, which opens the app store to search for something to handle the unknown protocol. Maybe we can do better than that, but I think we should followup that (it could be that we should just fix bug 1028091, or that we need android-specific code in docshell to cope with the lying external protocol service). Try: https://tbpl.mozilla.org/?tree=Try&rev=b961cb3a026c (third time lucky, I hope...)

Comment 31

[:Gijs (he/him)]

Attachment: allow fixing up URIs with schemes that we can't handle,

This time with the pref set in the test so it doesn't orange all over desktop...

Comment 32

[:Gijs (he/him)]

Try push #4: https://tbpl.mozilla.org/?tree=Try&rev=2a0188b99a15

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8444419 [details] [diff] [review]
allow fixing up URIs with schemes that we can't handle,

You don't want to add the bool var cache multiple times.

Comment 34

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #33)
> Comment on attachment 8444419 [details] [diff] [review]
> allow fixing up URIs with schemes that we can't handle,
> 
> You don't want to add the bool var cache multiple times.

Ugh. This is where my trusting existing code over my intuition / earlier experience when writing C++ is a bad thing, clearly:

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDefaultURIFixup.cpp#206

:-(

(I'll fix both up to use a guard)

Comment 35

[:Gijs (he/him)]

Attachment: allow fixing up URIs with schemes that we can't handle,

Try: https://tbpl.mozilla.org/?tree=Try&rev=087ac2b47b09

Comment 36

[Boris Zbarsky [:bzbarsky]]

Thank you!  That existing code is basically a memory leak!

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8445128 [details] [diff] [review]
allow fixing up URIs with schemes that we can't handle,

r=me

Comment 38

[:Gijs (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/930f107f460a

Comment 39

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

https://hg.mozilla.org/mozilla-central/rev/930f107f460a

Comment 40

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #29)
> Comment on attachment 8443061 [details] [diff] [review]
> allow fixing up URIs with schemes that we can't handle,
> 
> Boriss, I'm not fixing e.g. "60/5" in this bug, so you may or may not want
> to undo that duplicate. As I tried to make clear when I needinfo'd vt about
> this (who then seemed to agree that the case I've focused on is the main
> issue), for bugs like this, from an engineering perspective, it would be
> really helpful if there were clear steps to reproduce. Clearly, not every
> error page should be converted to a web search ("Firefox can't find the
> server for www.google.com" would be a fun one, for instance). I'm not really
> sure where UX feels the line is, and there are definitely cases that I'm not
> aware of (e.g. the 60/5 which was only mentioned in the now-duped-bug).
> 
> This bug only fixes the "thisisnotarealprotocol:stuff" case. It would
> probably make sense to file separate bugs if there are other cases where you
> think we should always do a keyword search, and to give concrete STR and
> expected/actual results there.

This time with needinfo, as I'm marking this verified...

Comment 41

[Jennifer Morrow [:Boriss] (UX)]

(In reply to :Gijs Kruitbosch from comment #40)
> (In reply to :Gijs Kruitbosch from comment #29)
> > Comment on attachment 8443061 [details] [diff] [review]
> > allow fixing up URIs with schemes that we can't handle,
> > 
> > Boriss, I'm not fixing e.g. "60/5" in this bug, so you may or may not want
> > to undo that duplicate. As I tried to make clear when I needinfo'd vt about
> > this (who then seemed to agree that the case I've focused on is the main
> > issue), for bugs like this, from an engineering perspective, it would be
> > really helpful if there were clear steps to reproduce. Clearly, not every
> > error page should be converted to a web search ("Firefox can't find the
> > server for www.google.com" would be a fun one, for instance). I'm not really
> > sure where UX feels the line is, and there are definitely cases that I'm not
> > aware of (e.g. the 60/5 which was only mentioned in the now-duped-bug).
> > 
> > This bug only fixes the "thisisnotarealprotocol:stuff" case. It would
> > probably make sense to file separate bugs if there are other cases where you
> > think we should always do a keyword search, and to give concrete STR and
> > expected/actual results there.
> 
> This time with needinfo, as I'm marking this verified...

Hey Gijs, could I possibly grab a build from you with patch applied?

Comment 42

[:Gijs (he/him)]

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #41)
> Hey Gijs, could I possibly grab a build from you with patch applied?

Hi, well, it's landed on Nightly a while ago, so any recent Nightly (last few days) should do!

Comment 43

[Sylvestre Ledru [:Sylvestre]]

Just like bug 693808.

Added in the 33 release notes with the wording "Improved search experience through the location bar"
Hoping to have a blog post to point to.