1048857 - Input like "http:// fgfg" kills UnifiedComplete

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

I just broke UnifiedComplete with entering "http:// fgfg" in the URL bar. The following exception gets logged:


[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.getURIForKeyword]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://gre/components/UnifiedComplete.js :: Search.prototype.execute< :: line 656"  data: no] UnifiedComplete.js:1300

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Can you elaborate on the visible effects of "kills" and "broke"?

Comment 2

[Marco Bonardo [:mak]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #1)
> Can you elaborate on the visible effects of "kills" and "broke"?

basically we stop searching, the autocomplete popup stops updating an keeps showing the results for http.//. But only for that search. It's not "broken" forever, it's not working just for that search.

Comment 3

[Marco Bonardo [:mak]]

the most interesting thing here (and the reason it should be fixed) is that we are generating an empty token, "http:// fgfg" will become ["", "fgfg"]

That is clearly broken and might create more subtle bugs than this.

Comment 4

[Marco Bonardo [:mak]]

since it would be wrong to remove tokens (other code relies on tokens count), I think the right solution here is to make stripPrefix a no-op if the passed-in string contains a space. that way we should have proper tokens.
Adding a test should be easy too.

Comment 5

[:Gijs (he/him)]

Attachment: Patch v1

This works in that it no longer throws an exception and also produces correct results. I've also changed it so that at the point where you type just 'www.' it doesn't suddenly go back to not suggesting anything based on that string anymore (which I thought was also wrong). The test passes (as do all the existing ones).

However... I question whether the results from the tests are what you would expect - in fact, I would have thought that if you typed 'http:// mo' and the only result was 'http://www.mozilla.org/', and that this would be reflected in at least one of autofilled or completed, but it is not.

Trying this out in practice in the location bar does see the second token matching later parts of the URL as expected, so I'm not sure what's going on here.

I'd ask Marco for review, but he's on PTO. Paolo?

Comment 6

[:Paolo Amadini]

Comment on attachment 8477018 [details] [diff] [review]
Patch v1

(In reply to :Gijs Kruitbosch from comment #5)
> Trying this out in practice in the location bar does see the second token
> matching later parts of the URL as expected, so I'm not sure what's going on
> here.

I tested on a new profile and, in line with the test cases, strangely it seems that the tokens don't match.

With this URL in history:
  https://www.mozilla.org/en-US/firefox/nightly/firstrun/

Typing in the URL bar:
  "https://mo" => Domain autocompleted, URL at second position in dropdown
  "https://ni" => URL at first position in dropdown
  "https:// mo" => No matches
  "https:// ni" => No matches

Type the URL above in the search box, close the results tab. Then:

  "https://mo" => Domain autocompleted, URL at second position in dropdown
  "https://ni" => URL at first position in dropdown, Google search visible
  "https:// mo" => Matches Google search only
  "https:// ni" => Matches Google search only

(The Google search is an URL with "https://" as part of its path.)

Comment 7

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #6)
> Comment on attachment 8477018 [details] [diff] [review]
> Patch v1
> 
> (In reply to :Gijs Kruitbosch from comment #5)
> > Trying this out in practice in the location bar does see the second token
> > matching later parts of the URL as expected, so I'm not sure what's going on
> > here.
> 
> I tested on a new profile and, in line with the test cases, strangely it
> seems that the tokens don't match.

Does this really need to be addressed as part of this bug? It seems like an orthogonal problem if tokens aren't being matched at all, because the correct tokens are being fed into the relevant API, as I understand it (see also comment #4).

If you think it does need to be fixed here, can you provide some idea of where to look for what's going wrong here? I know virtually nothing about places, much less autocomplete, and so I have approximately 0 idea of where to start looking for this issue.

Comment 8

[:Paolo Amadini]

While I'm not really as familiar with this code as Marco is, it seems to me that, despite comment 4, which I had read before, we shouldn't ignore the prefix removal when a space is present.

Apparently, per previous testing, the "https://" wouldn't match any URL if used as a search token. Not removing it when a space is present would break searches like "https://mo ni", which currently obtain results.

Simply removing the first "empty" token seems to fix the issue better for me. I added a "trim" here:

  this._searchString = fixupSearchText(this._trimmedOriginalSearchString.toLowerCase()).trim();

I haven't checked if the change above causes other failures in related code, in which case they should be investigated.

We'd need new tests for the "https://mo ni" case, since this wasn't caught as a regression.

Comment 9

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #8)
> I haven't checked if the change above causes other failures in related code,
> in which case they should be investigated.

To clarify, Marco mentioned changing the token count might have an effect, but I'm not sure what it is.

Comment 10

[:Paolo Amadini]

And existing tests in toolkit/components/places/tests/unifiedcomplete/ pass, so I believe we might proceed with the change and let Marco comment later if there is something the current automated tests have missed.

Comment 11

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #8)
> While I'm not really as familiar with this code as Marco is, it seems to me
> that, despite comment 4, which I had read before, we shouldn't ignore the
> prefix removal when a space is present.
> 
> Apparently, per previous testing, the "https://" wouldn't match any URL if
> used as a search token. Not removing it when a space is present would break
> searches like "https://mo ni", which currently obtain results.
> 
> Simply removing the first "empty" token seems to fix the issue better for
> me. I added a "trim" here:
> 
>   this._searchString =
> fixupSearchText(this._trimmedOriginalSearchString.toLowerCase()).trim();
> 

This doesn't seem to change the outcome for the automated tests I added (ie they pass without any changes from the patch that's on the bug)? How come the tests don't reflect what's in the location bar autocomplete popup? :-\

Comment 12

[Marco Bonardo [:mak]]

we don't match http:// (or other schemes) cause when matching on uri we used a "fixed" spec with trimmed scheme (see http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/SQLFunctions.cpp#383)

Changing the number of tokens would lie to the autocomplete code that relies on that count to figure which path to take. For example here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/UnifiedComplete.js#672
with just one token we might try to autoFill a search engine when instead we originally had 2 tokens thus we already know we can't match. so "https:// go" could match http://google.com (but not autoFill it).
Other code points also do a "this._searchTokens.length" > 0 check, I suspect this woulnd't be affected much in functionality. But I'm still worried about future changes that might assume token number is "right".

My assumption is that by keeping the https:// token we are not regressing anything from the status quo. The fact the status quo is not the best thing we can do is imo something to handle apart. We should figure what's the expected behavior when one of the tokens is a scheme, if we want it to enforce the scheme we must change the autocomplete_match function to do so. But that's more work than it's required here.

Comment 13

[:Paolo Amadini]

OK, so in order not to regress the "https://mo ni" case (for which we still need tests), the solution seems to be that we shouldn't strip the prefix when it is followed by a space, but we should strip it even if the string contains a space later. This prevents the first "token" from being empty.

Note that, when there are consecutive spaces in the string, it's "perfectly normal" for the autocomplete code to have empty tokens in the array. Just, the first token should not be empty.

(I definitely think there could be some useful refactorings here, but this is undoubtedly outside of the scope of this bug.)

(In reply to :Gijs Kruitbosch from comment #11)
> This doesn't seem to change the outcome for the automated tests I added (ie
> they pass without any changes from the patch that's on the bug)? How come
> the tests don't reflect what's in the location bar autocomplete popup? :-\

I'll be glad to try and help understanding the reason here...

Comment 14

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #13)
> OK, so in order not to regress the "https://mo ni" case (for which we still
> need tests), the solution seems to be that we shouldn't strip the prefix
> when it is followed by a space, but we should strip it even if the string
> contains a space later.

I'm not sure what this is saying.


> (In reply to :Gijs Kruitbosch from comment #11)
> > This doesn't seem to change the outcome for the automated tests I added (ie
> > they pass without any changes from the patch that's on the bug)? How come
> > the tests don't reflect what's in the location bar autocomplete popup? :-\
> 
> I'll be glad to try and help understanding the reason here...

Well, for starters, what do "autoFilled" and "completed" actually correspond to, UI-wise?

I should also note that, as far as I can tell, comment #12 and comment #13 disagree with each other, so I'm not really sure what the desired solution is anymore. :-\

Comment 15

[:Gijs (he/him)]

Orthogonally, from a user perspective, I've often tried to use:

"https foo"

to get only secure hits on foo.com/org/whatever.

It seems that with the new unifiedcomplete backend, this doesn't work. Is there a separate bug about this or should we be trying to fix that here, too?

Comment 16

[:Gijs (he/him)]

Attachment: Patch v1.1

This only avoids stripping on tokens that would otherwise be empty.

This still doesn't create matches unless the URI contains the protocol later on, but AIUI we don't want to fix this right now?

I will just note that that behaviour would still be a regression as compared to Firefox 32 beta, where e.g. "https:// mozilla" just gets me results that contain mozilla (potentially also insecure (ie http:) ones!)

Comment 17

[:Paolo Amadini]

Comment on attachment 8479965 [details] [diff] [review]
Patch v1.1

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

(In reply to :Gijs Kruitbosch from comment #16)
> This still doesn't create matches unless the URI contains the protocol later
> on, but AIUI we don't want to fix this right now?

Yeah, this is my interpretation.

::: toolkit/components/places/tests/unifiedcomplete/test_avoid_stripping_to_empty_tokens.js
@@ +4,5 @@
> +
> +add_task(function* test_protocol_trimming() {
> +  for (let prot of ["http", "https", "ftp"]) {
> +    let visit = {
> +      uri: NetUtil.newURI(prot + "://www.mozilla.org/test/?q=" + prot + encodeURIComponent("://") + "www.foo"),

So, I'd add a comment to explain why we include the protocol in the query part of the URI. This might not be necessary once we fix the follow-up bug.

@@ +27,5 @@
> +      "www.mo te"
> +    ];
> +    for (let input of inputs) {
> +      do_log_info("Searching for: " + input);
> +      let getsMatches = input.contains("mo");

getsMatches is probably a leftover, since all results in this test will match.

Comment 18

[:Paolo Amadini]

Ah, and the "dump" statement too is a leftover.

Comment 19

[:Gijs (he/him)]

w/ issues addressed,

remote:   https://hg.mozilla.org/integration/fx-team/rev/c7919e13cafb

Filed bug 1059395 for the results issue.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-bc orange.
https://hg.mozilla.org/integration/fx-team/rev/2cab6b69b74c

https://tbpl.mozilla.org/php/getParsedLog.php?id=46871209&tree=Fx-Team

Comment 21

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #20)
> Backed out for mochitest-bc orange.
> https://hg.mozilla.org/integration/fx-team/rev/2cab6b69b74c
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=46871209&tree=Fx-Team

Ugh. Sorry. Relanded with a small change:

remote:   https://hg.mozilla.org/integration/fx-team/rev/f3d15f941c58


which makes that test pass, without breaking the existing tests (verified locally).

Comment 22

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

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

Comment 23

[Andrei Vaida [:avaida]]

Verified fixed on Nightly 34.0a1 (2014-08-31) using Windows 7 64-bit, Ubuntu 14.04 LTS 64-bit and Mac OS X 10.8.5. Testing was based on Comment 0 and Comment 6.

Acceptance criteria for this patch:
- UnifiedComplete.js should no longer throw exceptions if the suggestions pane is trying to show entries for input similar to 'http:// ' (w/o apostrophe and w/ space).
- The suggestions pane should at least show the searches performed for keywords including 'http:// ' and 'https:// ' (w/o apostrophe and w/ space), until Bug 1059395 gets fixed.

Comment 24

[Mihaela Velimiroviciu (:mihaelav)]

Setting in-testsuite+ as the patch for this bug contains a test.

Comment 25

[:Gijs (he/him)]

Attachment: stuff

Comment 26

[:Gijs (he/him)]

Comment on attachment 8495210 [details] [diff] [review]
stuff

(Apologies, nothing to see here)