Closed
Bug 1048857
Opened 10 years ago
Closed 10 years ago
Input like "http:// fgfg" kills UnifiedComplete
Categories
(Toolkit :: Places, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox34 | --- | verified |
People
(Reporter: Unfocused, Assigned: Gijs)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Paolo
:
review+
|
Details | Diff | Splinter Review |
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
Updated•10 years ago
|
Flags: firefox-backlog+
Updated•10 years ago
|
Points: --- → 3
Updated•10 years ago
|
QA Contact: andrei.vaida
Comment 1•10 years ago
|
||
Can you elaborate on the visible effects of "kills" and "broke"?
Flags: needinfo?(bmcbride)
Updated•10 years ago
|
QA Whiteboard: [qa+]
Comment 2•10 years ago
|
||
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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.
Flags: needinfo?(bmcbride)
Comment 3•10 years ago
|
||
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•10 years ago
|
||
suggestedfix |
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.
Updated•10 years ago
|
Flags: in-testsuite?
Updated•10 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 34.3
QA Whiteboard: [qa+]
Flags: qe-verify+
Assignee | ||
Comment 5•10 years ago
|
||
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?
Attachment #8477018 -
Flags: review?(paolo.mozmail)
Comment 6•10 years ago
|
||
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.)
Attachment #8477018 -
Flags: review?(paolo.mozmail)
Assignee | ||
Comment 7•10 years ago
|
||
(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.
Flags: needinfo?(paolo.mozmail)
Comment 8•10 years ago
|
||
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.
Flags: needinfo?(paolo.mozmail)
Comment 9•10 years ago
|
||
(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•10 years ago
|
||
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.
Assignee | ||
Comment 11•10 years ago
|
||
(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? :-\
Flags: needinfo?(paolo.mozmail)
Comment 12•10 years ago
|
||
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•10 years ago
|
||
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...
Flags: needinfo?(paolo.mozmail)
Assignee | ||
Comment 14•10 years ago
|
||
(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. :-\
Assignee | ||
Comment 15•10 years ago
|
||
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?
Assignee | ||
Comment 16•10 years ago
|
||
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!)
Attachment #8477018 -
Attachment is obsolete: true
Attachment #8479965 -
Flags: review?(paolo.mozmail)
Comment 17•10 years ago
|
||
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.
Attachment #8479965 -
Flags: review?(paolo.mozmail) → review+
Comment 18•10 years ago
|
||
Ah, and the "dump" statement too is a leftover.
Assignee | ||
Comment 19•10 years ago
|
||
w/ issues addressed,
remote: https://hg.mozilla.org/integration/fx-team/rev/c7919e13cafb
Filed bug 1059395 for the results issue.
Whiteboard: [fixed-in-fx-team]
Comment 20•10 years ago
|
||
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
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Comment 21•10 years ago
|
||
(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).
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Comment 23•10 years ago
|
||
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.
Status: RESOLVED → VERIFIED
status-firefox34:
--- → verified
Comment 24•10 years ago
|
||
Setting in-testsuite+ as the patch for this bug contains a test.
Flags: in-testsuite? → in-testsuite+
Comment hidden (obsolete) |
Comment hidden (obsolete) |
You need to log in
before you can comment on or make changes to this bug.
Description
•