1378727 - Extension.jsm's promiseLocales method does main thread IO

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P1)

Description

[Florian Quèze [:florian]]

This profile shows an expensive nsJAR::Open call: https://perfht.ml/2sPsQp6

I think this is http://searchfox.org/mozilla-central/rev/5e1e8d2f244bd8c210a578ff1f65c3b720efe34e/toolkit/components/extensions/Extension.jsm#394

This code has a 'FIXME: We need a way to do this without main thread IO.' comment.

Would it make sense to re-use here the approach used in bug 1149732?

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1378727: Part 2 - Use the zip reader cache when reading extension locales.

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

Comment 2

[Andrew Swan [:aswan]]

Comment on attachment 8884587 [details]
Bug 1378727: Part 2 - Use the zip reader cache when reading extension locales.

https://reviewboard.mozilla.org/r/155474/#review161682

This looks good but we've had a slew of bugs in the addons manager about the jar cache (is that the same as the zip cache?) breaking flows where a developer is repeatedly writing an xpi to some path and then trying to install it, but the browser processes an old cached version.  Is this going to lead to similar issues?

Comment 3

[Kris Maglione [:kmag]]

(In reply to Andrew Swan [:aswan] from comment #2)
> This looks good but we've had a slew of bugs in the addons manager about the
> jar cache (is that the same as the zip cache?)

Yes.

> breaking flows where a developer is repeatedly writing an xpi to some path
> and then trying to install it, but the browser processes an old cached
> version.  Is this going to lead to similar issues?

No. We already use the JAR cache extensively when reading extension contents.
This is just a corner case where we currently don't.

Comment 4

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8809f15b58213f3292b9eaf7fa9583377d1fa30
Bug 1378727: Use the zip reader cache when reading extension locales. r=aswan

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing browser_ext_browserAction_context.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c0a02ed5bad838685ee03e245df89b608013fbb

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a8809f15b58213f3292b9eaf7fa9583377d1fa30&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=113782716&repo=mozilla-inbound

[task 2017-07-12T20:57:53.734021Z] 20:57:53     INFO - TEST-START | browser/components/extensions/test/browser/browser_ext_browserAction_context.js
[task 2017-07-12T20:57:53.900533Z] 20:57:53     INFO - GECKO(1101) | 1499893073893	addons.webextension.<unknown>	ERROR	Loading extension 'null': Reading manifest: Value for "default_locale" property must correspond to a directory in "_locales/". Not found: "_locales/en/"
[task 2017-07-12T20:57:53.905849Z] 20:57:53     INFO - GECKO(1101) | Extension startup failed: Extension contains packaging errors
[task 2017-07-12T20:57:53.914719Z] 20:57:53     INFO - GECKO(1101) | undefined
[task 2017-07-12T20:57:53.915065Z] 20:57:53     INFO - TEST-INFO | started process screentopng
[task 2017-07-12T20:57:55.028153Z] 20:57:55     INFO - TEST-INFO | screentopng: exit 0
[task 2017-07-12T20:57:55.033002Z] 20:57:55     INFO - Buffered messages logged at 20:57:53
[task 2017-07-12T20:57:55.034754Z] 20:57:55     INFO - Entering test bound testTabSwitchContext
[task 2017-07-12T20:57:55.037710Z] 20:57:55     INFO - Extension loaded
[task 2017-07-12T20:57:55.045207Z] 20:57:55     INFO - Buffered messages finished
[task 2017-07-12T20:57:55.049982Z] 20:57:55     INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_browserAction_context.js | Uncaught exception - startup failed
[task 2017-07-12T20:57:55.052149Z] 20:57:55     INFO - Leaving test bound testTabSwitchContext
[task 2017-07-12T20:57:55.055743Z] 20:57:55     INFO - Entering test bound testDefaultTitle
[task 2017-07-12T20:57:55.057878Z] 20:57:55     INFO - Extension loaded
[task 2017-07-12T20:57:55.064809Z] 20:57:55     INFO - Console message: [JavaScript Error: "1499893073893	addons.webextension.<unknown>	ERROR	Loading extension 'null': Reading manifest: Value for "default_locale" property must correspond to a directory in "_locales/". Not found: "_locales/en/"" {file: "resource://gre/modules/Log.jsm" line: 752}]
[task 2017-07-12T20:57:55.067330Z] 20:57:55     INFO - App_append@resource://gre/modules/Log.jsm:752:9
[task 2017-07-12T20:57:55.069523Z] 20:57:55     INFO - log@resource://gre/modules/Log.jsm:390:7
[task 2017-07-12T20:57:55.073335Z] 20:57:55     INFO - error@resource://gre/modules/Log.jsm:398:5
[task 2017-07-12T20:57:55.075411Z] 20:57:55     INFO - packagingError@resource://gre/modules/Extension.jsm:352:5
[task 2017-07-12T20:57:55.077555Z] 20:57:55     INFO - manifestError@resource://gre/modules/Extension.jsm:346:5
[task 2017-07-12T20:57:55.081306Z] 20:57:55     INFO - initAllLocales@resource://gre/modules/Extension.jsm:699:9
[task 2017-07-12T20:57:55.085394Z] 20:57:55     INFO - async*_receiveMessageAPI/<@chrome://mochikit/content/tests/SimpleTest/SpecialPowersObserverAPI.js:592:20
[task 2017-07-12T20:57:55.088486Z] 20:57:55     INFO - promise callback*_receiveMessageAPI@chrome://mochikit/content/tests/SimpleTest/SpecialPowersObserverAPI.js:590:9
[task 2017-07-12T20:57:55.090583Z] 20:57:55     INFO - ChromePowers.prototype._receiveMessage@chrome://mochikit/content/tests/SimpleTest/ChromePowers.js:83:14
[task 2017-07-12T20:57:55.092805Z] 20:57:55     INFO - ChromePowers.prototype._sendAsyncMessage@chrome://mochikit/content/tests/SimpleTest/ChromePowers.js:45:3
[task 2017-07-12T20:57:55.094932Z] 20:57:55     INFO - startup@chrome://mochikit/content/tests/SimpleTest/specialpowersAPI.js:1980:9
[task 2017-07-12T20:57:55.098210Z] 20:57:55     INFO - runTests@chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_browserAction_context.js:140:9
[task 2017-07-12T20:57:55.102899Z] 20:57:55     INFO - async*testTabSwitchContext@chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_browserAction_context.js:148:9
[task 2017-07-12T20:57:55.105002Z] 20:57:55     INFO - Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:763:21
[task 2017-07-12T20:57:55.106948Z] 20:57:55     INFO - TaskImpl_run@resource://gre/modules/Task.jsm:331:42
[task 2017-07-12T20:57:55.110001Z] 20:57:55     INFO - TaskImpl@resource://gre/modules/Task.jsm:280:3
[task 2017-07-12T20:57:55.111913Z] 20:57:55     INFO - asyncFunction@resource://gre/modules/Task.jsm:252:14
[task 2017-07-12T20:57:55.113901Z] 20:57:55     INFO - Task_spawn@resource://gre/modules/Task.jsm:166:12
[task 2017-07-12T20:57:55.120086Z] 20:57:55     INFO - Tester_execTest@chrome://mochikit/content/browser-test.js:758:9
[task 2017-07-12T20:57:55.122180Z] 20:57:55     INFO - Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:670:7
[task 2017-07-12T20:57:55.124230Z] 20:57:55     INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
[task 2017-07-12T20:57:55.125957Z] 20:57:55     INFO -

Comment 6

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90ae2fcd089a5bf901292c282a98187fed4c79a2
Bug 1378727: Use the zip reader cache when reading extension locales. r=aswan

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d259c59353d307f80f0d4c0ee37e73cab13f7a03
Backed out changeset 90ae2fcd089a (bug 1378727) for numerous xpcshell test failures on Windows, starting with test_ext_browsingData_downloads.js.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Backed out again for Windows-specific xpcshell test failures.  See:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=xpcshell%20windows&fromchange=633c98dc992f&tochange=a4490a86f11f

(Sorry, can't needinfo since there's already one.)

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And, actually, what the first test failure was seemed to vary a bit.

Comment 10

[Kris Maglione [:kmag]]

Attachment: Bug 1378727: Part 1 - Add helper to enumerate cached ZipReader without locking issues.

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

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8884587 [details]
Bug 1378727: Part 2 - Use the zip reader cache when reading extension locales.

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

Comment 12

[Andrew Swan [:aswan]]

Comment on attachment 8886314 [details]
Bug 1378727: Part 1 - Add helper to enumerate cached ZipReader without locking issues.

https://reviewboard.mozilla.org/r/157060/#review162556

Logically this seems sound but somebody who knows all the intricacies of mozilla-flavored c++ should take a look.

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 8886314 [details]
Bug 1378727: Part 1 - Add helper to enumerate cached ZipReader without locking issues.

https://reviewboard.mozilla.org/r/157060/#review162568

::: toolkit/mozapps/extensions/AddonManagerStartup.cpp:258
(Diff revision 1)
>  }
>  
> +static Result<nsCOMPtr<nsIZipReaderCache>, nsresult>
> +GetJarCache()
> +{
> +  RefPtr<nsIIOService> ios = services::GetIOService();

This should be an nsCOMPtr

Comment 14

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41c8259730b58d64d25cb7e49f46ea82238058ab
Bug 1378727: Part 1 - Add helper to enumerate cached ZipReader without locking issues. r=Mossop

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7c01e9f4caa1221ee309d428d68882ffee55aa5
Bug 1378727: Part 2 - Use the zip reader cache when reading extension locales. r=aswan

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/41c8259730b5
https://hg.mozilla.org/mozilla-central/rev/a7c01e9f4caa