Extension.jsm's promiseLocales method does main thread IO

RESOLVED FIXED in Firefox 56

Status

enhancement
P1
normal
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: florian, Assigned: kmag)

Tracking

(Blocks 2 bugs)

unspecified
mozilla56
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [qf])

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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?
(Assignee)

Updated

2 years ago
Whiteboard: [qf]
(Assignee)

Updated

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

Comment 2

2 years ago
mozreview-review
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?
Attachment #8884587 - Flags: review?(aswan) → review+
(Assignee)

Comment 3

2 years ago
(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.

Updated

2 years ago
Priority: -- → P1
(Assignee)

Comment 4

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8809f15b58213f3292b9eaf7fa9583377d1fa30
Bug 1378727: Use the zip reader cache when reading extension locales. r=aswan
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 -
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/90ae2fcd089a5bf901292c282a98187fed4c79a2
Bug 1378727: Use the zip reader cache when reading extension locales. r=aswan
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.
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.)
And, actually, what the first test failure was seemed to vary a bit.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Flags: needinfo?(kmaglione+bmo)

Comment 12

2 years ago
mozreview-review
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.
Attachment #8886314 - Flags: review?(aswan)
(Assignee)

Updated

2 years ago
Attachment #8886314 - Flags: review?(dtownsend)

Comment 13

2 years ago
mozreview-review
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
Attachment #8886314 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 14

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/41c8259730b5
https://hg.mozilla.org/mozilla-central/rev/a7c01e9f4caa
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Updated

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