Build ID XUL: Mozilla/5.0 (Android; Linux armv7l; rv:12.0a1) Gecko/20120125 Firefox/12.0a1 Fennec/12.0a1 Device: Samsung Galaxy Nexus OS: Android 4.0 Steps to reproduce: 1. Install several add-ons - restart app if requested 2. Go to Add-on Manager 3. Go to About:fennec 4. Check for updates- install update 5. Go to Add-on Manager 6. Kill application 7. open app Nightly Expected Result: 1. After step 2 and step 5 the installed add-ons should be visible Actual Result: 1. Add-ons are visible only after restart of the app. As it can be seen from steps to reproduce this behavior is both on XUL and Native versions.

Sounds like bug 717904, but the new information is that this happens for XUL too. Is it possible to get a regression range for this bug? I assume the restarting after an add-on install worked at some point for XUL Fennec.

This is not a dupe. This is a bug in profile migration from XUL to Native.

"As it can be seen from steps to reproduce this behavior is both on XUL and Native versions." This comment made me think otherwise

XUL Fennec add-ons will never "Just work" in Native Fennec, and will always be disabled.

Which add-ons were installed?

Add-ons: - Adblock Plus, - Reading list - Clear Mobile History - Bigger test

Wes - can you look at this after the higher priority stuff in your queue is done?

Stealing this bug and nominating for blocking. From bug 721782 comment 10: I can still reproduce this. It looks like a very similar problem to bug 717904; I get the same errors on startup: E/GeckoConsole( 8350): [JavaScript Error: "ERROR addons.xpi: Failed to open database (1st attempt): [Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]" nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)" location: "JS frame :: resource://gre/modules/XPCOMUtils.jsm :: XPCU_serviceLambda :: line 232" data: no]" {file: "resource://gre/modules/XPCOMUtils.jsm" line: 232}] E/GeckoConsole( 8350): [JavaScript Error: "ERROR addons.xpi: Failed to open database (2nd attempt): TypeError: Services.storage is undefined" {file: "resource://gre/modules/XPIProvider.jsm" line: 4241}] E/GeckoConsole( 8350): [JavaScript Error: "ERROR addons.xpi: Error during startup file checks, rolling back any database changes: TypeError: Services.storage is undefined" {file: "resource://gre/modules/XPIProvider.jsm" line: 4249}] E/GeckoConsole( 8350): [JavaScript Error: "ERROR addons.xpi: Error creating statement rollbackSavepoint (ROLLBACK TO SAVEPOINT 'default')" {file: "resource://gre/modules/XPIProvider.jsm" line: 4604}] E/GeckoConsole( 8350): [JavaScript Error: "ERROR addons.manager: Exception calling provider startup: TypeError: this.connection is undefined" {file: "resource://gre/modules/XPIProvider.jsm" line: 4601}] Next step: Running under a debugger to see why the storage service is failing to initialize.

As far as I can tell, the ProfileMigrator/SQLiteBridge code is causing sqlite3_initialize to get called before the storage service is initialized. This breaks nsStorageService, just like in bug 717904 comment 18. The underlying issue is bug 730495. As a workaround for Fennec 1.0, we could either delay profile migration until after Gecko is initialized, or we could restart the browser after profile migration. Any thoughts, Gian-Carlo?

I'm not sure either is feasible. In principle, there isn't much against slightly delaying the run of Profile Migration, but we just did the work to make Sync call into Profile Migration as long as the entire history DB isn't migrated yet. I don't think Sync can reasonably make any guarantees about Gecko startup, nor does it make any sense to have it do so. I'm going to defer to rnewman here to see if we can have lifetimes that mutually make sense.

(In reply to Gian-Carlo Pascutto (:gcp) from comment #10) > I'm going to defer to rnewman here to see if we can have lifetimes that > mutually make sense. Am I right in my understanding of the problem? That SQLiteBridge can't even load safely before Gecko runs, because it doesn't call sqlite_config? That implies that Sync can't touch any CP that uses SQLiteBridge, which is currently "all of them apart from bookmarks and history". If that's the case, then it seems there are only two solutions: * Prior to using SQLiteBridge, check some pref. This will indicate whether Gecko has ever been launched. If false, launch a silent Gecko task that self-quits. Problems: maintaining the accuracy of that pref. * Fix Bug 730495. We can't load Gecko every time we touch a 'native' CP; that could be four or five times, every thirty seconds, all day. I don't really have enough context to speculate much more than that...

Because working around here by reordering Gecko startup is a real mess, what about the following approach: 1) The wrappers that are configured in SQLite *only* seem to handle the alignment issue with jemalloc: https://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageService.cpp#535 https://mxr.mozilla.org/mozilla-central/source/memory/mozalloc/mozalloc.cpp 2) http://www.sqlite.org/compile.html SQLite can be compiled to support such allocators, removing the need for wrappers: SQLITE_4_BYTE_ALIGNED_MALLOC So, if we add this SQLITE_4_BYTE_ALIGNED_MALLOC to sqlite's compile options, we can remove those wrappers (or only in Fennec) and the sqlite3_config call entirely. This only makes sense if sqlite3_initialize, sqlite3_vfs_register etc don't cause similar problems.

Is there any performance impact from enabling SQLITE_4_BYTE_ALIGNED_MALLOC?

The documentation suggests Windows uses this, so it can't be that bad. Pretty hard to tell from just grepping through the code, though. Maybe we should continue this part of the discussion in bug 730495.

(In reply to Matt Brubeck (:mbrubeck) from comment #13) > Is there any performance impact from enabling SQLITE_4_BYTE_ALIGNED_MALLOC? The SQLITE_4_BYTE_ALIGNED_MALLOC macro only effects assert() statements. So it should have zero impact on a production build (when assert()s are omitted.) SQLite contains a lot of assert()s that verify that various data structures are aligned on an 8-byte boundary, which is required on some platforms (ex: sparc) and improves performance on others (ex: x86_64). But on some systems, system malloc() only returns 4-byte aligned buffers instead of the more usual 8-byte aligned buffers. On such systems, we have to define SQLITE_4_BYTE_ALIGNED_MALLOC to prevent those alignment assert() statements from failing. Other than altering the behavior of those 19 or 20 assert() statements, SQLITE_4_BYTE_ALIGNED_MALLOC has no effect on the code.

This patch defines SQLITE_4_BYTE_ALIGNED_MALLOC and disables the sqlite3_config call, on Android only. Pushed to Try: https://tbpl.mozilla.org/?tree=Try&rev=e6c1a072528d

Try run for e6c1a072528d is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=e6c1a072528d Results (out of 224 total builds): success: 184 warnings: 40 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mbrubeck@mozilla.com-e6c1a072528d

I don't understand the patch honestly, if the problem is jemalloc, shouldn't you just temporarily disable using jemalloc on Android for SQLite? SQLITE_4_BYTE_ALIGNED_MALLOC, according to the description Richard kindly gave us in comment 15, has really nothing to do with our use of jemalloc, nor can be even considered a replacement for it. We use jemalloc cause it gives us less fragmentation and more precise measurement of the SQLite memory usage, not for alignment issues. What we may do is instead replacing MOZ_MEMORY with MOZ_STORAGE_MEMORY (or any other placeholder) and in the Makefile.in define rules for when we want it, so if (MOZ_MEMORY && !ANDROID) define MOZ_STORAGE_MEMORY

Provided this is intended as a workaround until Bug 730495 is properly fixed.

Just to clarify: >SQLITE_4_BYTE_ALIGNED_MALLOC, according to the description Richard kindly gave us >in comment 15, has really nothing to do with our use of jemalloc, nor can be even >considered a replacement for it. We use jemalloc cause it gives us less >fragmentation and more precise measurement of the SQLite memory usage, not for >alignment issues. jemalloc is *causing* the alignment issue, i.e. it actively breaks SQLite without the workaround that the current code is already doing. This in turn requires us to call sqlite3_config to *fix* that, which again causes bug 730495. >I don't understand the patch honestly, if the problem is jemalloc, shouldn't you >just temporarily disable using jemalloc on Android for SQLite? Why is this better? The only problem we have with jemalloc is the alignment issue, which SQLite can handle with this define. Disabling it entirely is a bit of "throwing the baby out with the bath water".

(In reply to Gian-Carlo Pascutto (:gcp) from comment #20) > jemalloc is *causing* the alignment issue, i.e. it actively breaks SQLite > without the workaround that the current code is already doing. This in turn > requires us to call sqlite3_config to *fix* that, which again causes bug > 730495. Sorry, I need more info, I'm not following what you say. sqlite3_config is used to tell sqlite we want to use our own allocator, that is based on jemalloc and we did that for the above reasons I posted, not for any alignment issue.

This patch addresses feedback from Marco's comment 18. With this patch, sqlite will no longer use jemalloc on Android. Obviously this is not desirable, so we would use it only as a temporary solution if bug 730495 can't be fixed soon.

Comment on attachment 614425 [details] [diff] [review] patch v2 (disable jemalloc for sqlite on Android) Review of attachment 614425 [details] [diff] [review]: ----------------------------------------------------------------- This is fine as long as it's temporary, cause it's creating some asymmetry. Indeed on Windows, our SQLite build assumes we use jemalloc, so it would fail (being unable to find a malloc_usable_size implementation) if anyone would ever try to disable jemalloc on Win Storage just changing this define. Just to be sure that's known, please add a comment in the Makefile, before your changes, pointing out that using the define to disable the jemalloc allocator on Win would also require special handling (disabling) of the MOZ_MEMORY stuff in db/sqlite3/src/Makefile.in. ::: storage/src/Makefile.in @@ +54,5 @@ > ifeq ($(OS_ARCH),Linux) > DEFINES += -DXP_LINUX > endif > > +# Don't enable jemalloc on Android because we can't guarantee that Gecko will s/jemalloc/the jemalloc allocator/

Landed with comment changes as requested: https://hg.mozilla.org/integration/mozilla-inbound/rev/3ae0296eb3b2 We need to watch for perf/footprint regressions caused by this change.

Verified fixed on Nightly 15.0a1 (2012-05-20) Samsung Galaxy SII (Android 2.3.4)