extension with multiple locales hangs firefox for a few minutes

VERIFIED FIXED in Firefox 49

Status

P1
normal
VERIFIED FIXED
2 years ago
2 months ago

People

(Reporter: mixedpuppy, Assigned: kmag)

Tracking

49 Branch
mozilla51
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox48 wontfix, firefox49 verified, firefox-esr45 unaffected, firefox50 verified, firefox51 verified)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
I have a web extension with 27 locale files, ranging from 100K to 200K each.  Installing the addon in the current beta (49b8) hangs firefox for at least a couple minutes, with 90+% cpu usage.
(Assignee)

Comment 1

2 years ago
Can you attach or send me a copy of the add-on?

We do load all locales when the extension is first installed, but we only do minimal processing on them, so I'm surprised it's that noticeable of a problem.
(Reporter)

Comment 2

2 years ago
To a lesser extent, using the addon manager to disable then enable the addon also produces a noticable delay.
(Assignee)

Updated

2 years ago
Assignee: nobody → kmaglione+bmo
Priority: -- → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 4

2 years ago
Comment on attachment 8786450 [details]
Bug 1299256: Fix performance issues when stripping comments from large locale files.

Approval Request Comment
[Feature/regressing bug #]: Bug 1196283
[User impact if declined]: This bug causes Firefox to hang, often for over a minute, when first installing an extension containing a large amount of locale data.
[Describe test coverage new/current, TreeHerder]: These changes affect the code which strips comments from JSON files. The behavior of that code is well-tested in all affected versions, but there are no tests for this particular issue, since it's the kind of performance issue which does not lend itself well to automated testing.
[Risks and why]: Low. This patch makes minor changes to a regular expression which improves its performance, but does not change its results for any valid input. It could potentially produce different results for invalid JSON with unterminated string literals, but that input would be rejected with or without this patch.
[String/UUID change made/needed]: None.
Attachment #8786450 - Flags: approval-mozilla-beta?
Attachment #8786450 - Flags: approval-mozilla-aurora?

Comment 5

2 years ago
mozreview-review
Comment on attachment 8786450 [details]
Bug 1299256: Fix performance issues when stripping comments from large locale files.

https://reviewboard.mozilla.org/r/75386/#review73568

Oh man... this is very subtle (and my head still hurts a bit, luckily we don't support the multiline comments, it is better if I don't even start to imagine what kind of "regular expression monster" we would use :-))

Anyway, if I read this change correctly:

we are explicitly adding the '\n' character to the chars that will invalidate a match, and this should make the processing of a big json locale file with comments faster

am I reading it right?

It would be nice to be able to have a test that ensures that we are able to load a reasonable big locale file in a reasonable amount of time over the time.

Anyway, I briefly tried locally to load some big file with the two regular expressions (before and after the fix) and it seems to confirm that this new one is faster (at least a bit faster of the previous one).
Attachment #8786450 - Flags: review?(lgreco) → review+
(Reporter)

Comment 6

2 years ago
testcase
Created attachment 8786857 [details]
sample_locales-1.xpi

This xpi is an example of a web extension with a full set of locale files, and which causes a long/heavy hang during installation, and a noticable hang when enabling (after disabling) in the addon manager.
(Assignee)

Comment 7

2 years ago
(In reply to Luca Greco [:rpl] from comment #5)
> we are explicitly adding the '\n' character to the chars that will
> invalidate a match, and this should make the processing of a big json locale
> file with comments faster

With or without comments. In fact, the performance is more of a problem in
files with more lines but fewer comments, since we currently match from the
start of each line until either the first comment, or the end of the file.

> It would be nice to be able to have a test that ensures that we are able to
> load a reasonable big locale file in a reasonable amount of time over the
> time.

It's not really doable. Timing in tests isn't reliable enough to handle
something like this.
(Assignee)

Comment 8

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/21ec43d33e0c2ac8049355c506282ce076625578
Bug 1299256: Fix performance issues when stripping comments from large locale files. r=rpl
(Assignee)

Comment 9

2 years ago
Shane, would you mind confirming this when the inbound build is done?
Flags: needinfo?(mixedpuppy)
(Assignee)

Updated

2 years ago
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox-esr45: --- → unaffected
(Reporter)

Comment 10

2 years ago
nice!  No noticeable delay when installing.
Flags: needinfo?(mixedpuppy)

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/21ec43d33e0c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment on attachment 8786450 [details]
Bug 1299256: Fix performance issues when stripping comments from large locale files.

This patch fixes a performance issue when installing extension with large locale files. Take it in aurora and 49 beta.
Attachment #8786450 - Flags: approval-mozilla-beta?
Attachment #8786450 - Flags: approval-mozilla-beta+
Attachment #8786450 - Flags: approval-mozilla-aurora?
Attachment #8786450 - Flags: approval-mozilla-aurora+

Updated

2 years ago
status-firefox48: affected → wontfix

Comment 13

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/d73202a4a0cd
status-firefox49: affected → fixed

Comment 14

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/a46345ccdae2
status-firefox50: affected → fixed
Flags: qe-verify+

Comment 15

2 years ago
This issue is verified as fixed on Firefox 51.0a1 (2016-09-02), Firefox 50.0a2 (2016-09-02), Firefox 49.0b9 unbranded build (20160901141622) under Windows 7 64-bit, Ubuntu 16.04 32-bit and Ubuntu 12.04 64-bit.

There is no hanging when the WebExtension is installed.
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified
status-firefox50: fixed → verified
status-firefox51: fixed → verified

Updated

2 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.