1414246 - Slow text input in the search field, when a tab with a large data URI is open

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Deac Alin]

Steps to reproduce:
1. On an existing profile, go to about:telemetry
2. At the bottom-left, click "Raw JSON"
-> A new tab opens with the data URI for the raw JSON.
3. Open new tab and start typing in the search field 
4. Close the data URI tab

Actual Results
3. While the Data Uri tab is active/present(from step 2), input text in the search field is slower than normal (it can be observed a delay)
The issue is also present when using the backspace key to remove the text in the Search field.

Expected Results
3. While the Data Uri tab is active/present(from step 2), input text in the search field should not be affected (performance wise)

4. Close the data URI tab
Text input performance reverts to normal

Note: Check the attached screen record of the issue. 
The issue is most visible on Mac 10.x and Windows 10, less on Ubuntu.

Comment 1

[Deac Alin]

Screen-record > https://www.youtube.com/watch?v=rf5zQUV7VGI&feature=youtu.be

Comment 2

[Florian Quèze [:florian]]

The problem seems to be with the 'Switch to Tab' item that displays the full data url before adding an ellipsis in there.
A profile (https://perfht.ml/2hCy2dv) shows we are spending most of the time in _handleOverflow.

Comment 3

[Marco Bonardo [:mak]]

Apart from bug 1356532, in general it doesn't make sense to render a giant string, when we know we'll crop it regardless. We could limit the text length in any case to a meaningful amount, so that rendering and overflow calculations may be cheaper.

Comment 4

[:Gijs (out for now; he/him)]

It seems unfortunately the fix from bug 907001 was not fully effective, because the text run limit is defined on the inputbox but needs to be on the popup.

I'm looking at whether there's any further improvements we can make here.

Comment 6

[:Gijs (out for now; he/him)]

Attachment: Bug 1414246 - actually use textRunsMaxLen to limit autocomplete text run length in address bar autocomplete,

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

Comment 7

[:Gijs (out for now; he/him)]

Attachment: Bug 1414246 - only create page-icon URLs for some schemes (avoiding long page-icon:data URIs),

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

Comment 8

[:Gijs (out for now; he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=236e09ce9be62841d41e4839e1405cafe93241d7

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8975450 [details]
Bug 1414246 - actually use textRunsMaxLen to limit autocomplete text run length in address bar autocomplete,

https://reviewboard.mozilla.org/r/243744/#review249576

::: toolkit/content/widgets/autocomplete.xml:2018
(Diff revision 1)
>            }
>  
>            if (Array.isArray(title)) {
>              // For performance reasons we may want to limit the title size.
>              if (popup.textRunsMaxLen) {
> -              title = title.map(t => t.substr(0, popup.textRunsMaxLen));
> +              title.forEach(t => t[0] = t[0].substr(0, popup.textRunsMaxLen));

Based on https://searchfox.org/mozilla-central/rev/a85db9e29eb3f022dbaf8b9a6390ecbacf51e7dd/toolkit/content/widgets/autocomplete.xml#1690-1691 I think we should be limiting the length of each even-numbered index of the array, not just the 0-index.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8975450 [details]
Bug 1414246 - actually use textRunsMaxLen to limit autocomplete text run length in address bar autocomplete,

https://reviewboard.mozilla.org/r/243744/#review249644

::: toolkit/content/widgets/autocomplete.xml:1949
(Diff revision 1)
>                displayUrl = input.trimValue(displayUrl);
>              displayUrl = this._unescapeUrl(displayUrl);
>            }
>            // For performance reasons we may want to limit the displayUrl size.
>            if (popup.textRunsMaxLen) {
> -            displayUrl = displayUrl.substr(0, popup.textRunsMaxLen);
> +            displayUrl = (displayUrl || "").substr(0, popup.textRunsMaxLen);

instead of substr on an empty string, you could just:
if (popup.textRunsMaxLen && displayUrl) {

::: toolkit/content/widgets/autocomplete.xml:2024
(Diff revision 1)
>              }
>              this._setUpEmphasisedSections(this._titleText, title);
>            } else {
>              // For performance reasons we may want to limit the title size.
>              if (popup.textRunsMaxLen) {
> -              title = title.substr(0, popup.textRunsMaxLen);
> +              title = (title || "").substr(0, popup.textRunsMaxLen);

ditto

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8975451 [details]
Bug 1414246 - only create page-icon URLs for some schemes (avoiding long page-icon:data URIs),

https://reviewboard.mozilla.org/r/243746/#review249648

::: toolkit/components/places/UnifiedComplete.js:308
(Diff revision 1)
>  // TODO (bug 1045924): use foreign_count once available.
>  const SQL_BOOKMARKED_URL_QUERY = urlQuery("AND bookmarked");
>  
>  const SQL_BOOKMARKED_TYPED_URL_QUERY = urlQuery("AND bookmarked AND h.typed = 1");
>  
> +const DEFAULT_FAVICON_URL = "chrome://mozapps/skin/places/defaultFavicon.svg";

Please use PlacesUtils.favicons.defaultFavicon where necessary, instead of this const. It's not an hot path so we should not even need to memoize it.

::: toolkit/components/places/UnifiedComplete.js:344
(Diff revision 1)
> +const kProtocolsWithIcons = kSchemesWithIcons.map(s => s + ":");
> +function iconHelper(url) {
> +  if (typeof url == "string") {
> +    for (let i = 0; i < kProtocolsWithIcons.length; i++) {
> +      if (url.startsWith(kProtocolsWithIcons[i])) {
> +        return "page-icon:" + url;

if (typeof url == "string") {
  return kProtocolsWithIcons.some(s => url.startsWith(s)) ?
    "page-icon:" + url : PlacesUtils.favicons.defaultFavicon;
}

some() should be more optimizeable than a bare loop

::: toolkit/components/places/UnifiedComplete.js:349
(Diff revision 1)
> +        return "page-icon:" + url;
> +      }
> +    }
> +    return DEFAULT_FAVICON_URL;
> +  }
> +  if (url && "href" in url && kProtocolsWithIcons.includes(url.protocol)) {

Is this check faster than instanceof URL?

::: toolkit/components/places/UnifiedComplete.js:352
(Diff revision 1)
> +    return DEFAULT_FAVICON_URL;
> +  }
> +  if (url && "href" in url && kProtocolsWithIcons.includes(url.protocol)) {
> +    return "page-icon:" + url.href;
> +  }
> +  if (url && "spec" in url && kSchemesWithIcons.includes(url.scheme)) {

is this check faster than instanceof Ci.nsIURI?

Fwiw, nothing seems to be passing an nsIURI, and I don't think we'll add further use of it, so maybe you could simplify the code by only providing the protocols (with ":")

Comment 12

[:Gijs (out for now; he/him)]

Comment on attachment 8975451 [details]
Bug 1414246 - only create page-icon URLs for some schemes (avoiding long page-icon:data URIs),

https://reviewboard.mozilla.org/r/243746/#review249904

::: toolkit/components/places/UnifiedComplete.js:349
(Diff revision 1)
> +        return "page-icon:" + url;
> +      }
> +    }
> +    return DEFAULT_FAVICON_URL;
> +  }
> +  if (url && "href" in url && kProtocolsWithIcons.includes(url.protocol)) {

I don't know. I've made it do an instanceof check instead. I'm always a bit wary of those because of the issue of different globals, but it looks like that isn't a practical issue with these builtins, maybe because of webidl.

::: toolkit/components/places/UnifiedComplete.js:352
(Diff revision 1)
> +    return DEFAULT_FAVICON_URL;
> +  }
> +  if (url && "href" in url && kProtocolsWithIcons.includes(url.protocol)) {
> +    return "page-icon:" + url.href;
> +  }
> +  if (url && "spec" in url && kSchemesWithIcons.includes(url.scheme)) {

Removed this.

Comment 13

[:Gijs (out for now; he/him)]

Comment on attachment 8975450 [details]
Bug 1414246 - actually use textRunsMaxLen to limit autocomplete text run length in address bar autocomplete,

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

Comment 14

[:Gijs (out for now; he/him)]

Comment on attachment 8975451 [details]
Bug 1414246 - only create page-icon URLs for some schemes (avoiding long page-icon:data URIs),

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

Comment 15

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 5f8a44aaf15b5c257f3510f22f7bdeaa9343b5ae -d 339ffc4c6651: rebasing 463538:5f8a44aaf15b "Bug 1414246 - actually use textRunsMaxLen to limit autocomplete text run length in address bar autocomplete, r=jaws"
merging browser/base/content/urlbarBindings.xml
merging toolkit/content/widgets/autocomplete.xml
rebasing 463539:972d63c04cee "Bug 1414246 - only create page-icon URLs for some schemes (avoiding long page-icon:data URIs), r=mak" (tip)
merging toolkit/components/places/UnifiedComplete.js
warning: conflicts while merging toolkit/components/places/UnifiedComplete.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 16

[:Gijs (out for now; he/him)]

Comment on attachment 8975450 [details]
Bug 1414246 - actually use textRunsMaxLen to limit autocomplete text run length in address bar autocomplete,

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

Comment 17

[:Gijs (out for now; he/him)]

Comment on attachment 8975451 [details]
Bug 1414246 - only create page-icon URLs for some schemes (avoiding long page-icon:data URIs),

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e25e096a786
actually use textRunsMaxLen to limit autocomplete text run length in address bar autocomplete, r=jaws
https://hg.mozilla.org/integration/autoland/rev/1a5e82c2b8fb
only create page-icon URLs for some schemes (avoiding long page-icon:data URIs), r=mak

Comment 19

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/3e25e096a786
https://hg.mozilla.org/mozilla-central/rev/1a5e82c2b8fb