Move database creation out of startup path

REOPENED
Unassigned

Status

()

Firefox for Android
Data Providers
REOPENED
6 years ago
3 years ago

People

(Reporter: wesj, Unassigned, NeedInfo)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 29
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec+)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 654318 [details] [diff] [review]
Patch

We create/upgrade signons.sqlite and formhistory.sqlite at startup on fist run or upgrade for sync. That can take anywhere for 1-3sec. We should move it to after first paint, specifically to improve webapp first run experience.
(Reporter)

Updated

6 years ago
Attachment #654318 - Attachment is patch: true
(Reporter)

Comment 1

6 years ago
Comment on attachment 654318 [details] [diff] [review]
Patch

Whoops. Please ignore that stupid comment. At least it doesn't say something embarrassing....
Attachment #654318 - Flags: review?(mark.finkle)
Comment on attachment 654318 [details] [diff] [review]
Patch

good first step to offloading some of the firstrun work
Attachment #654318 - Flags: review?(mark.finkle) → review+
(Reporter)

Comment 4

6 years ago
I backed this out on suspicion that our password provider tests rely on this happening early.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb414dc59262

Updated

6 years ago
Blocks: 789185
Duplicate of this bug: 789185
We should fix the problem and get this landed. This is 300ms or about 20% of our startup time.
Backed out again in http://hg.mozilla.org/integration/mozilla-inbound/rev/64a0ab78b261 for crashing in every reftest, crashtests and jsreftest, and timing out in startup (or, equally likely, crashing but talos doesn't know how to detect startup crashes) in every talos test.
Still a problem at ~250ms on a Galaxy Nexus.
Blocks: 807322
(Reporter)

Comment 13

4 years ago
Created attachment 8365282 [details] [diff] [review]
Patch v2

Unbitrotted (and updated). Tossed onto try as well:
https://tbpl.mozilla.org/?tree=Try&rev=dd7a68a312ad
Attachment #654318 - Attachment is obsolete: true
Comment on attachment 8365282 [details] [diff] [review]
Patch v2

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+    // Store the low-precision buffer pref {

'{' at end of comment is a typo

>     this.gUseLowPrecision = Services.prefs.getBoolPref("layers.low-precision-buffer");
>-
>     // notify java that gecko has loaded

nit: keep the blank line for readability

>   onAppUpdated: function() {
>-    // initialize the form history and passwords databases on upgrades
>-    Services.obs.notifyObservers(null, "FormHistory:Init", "");
>-    Services.obs.notifyObservers(null, "Passwords:Init", "");
>-

Why not add the "DOMContentLoaded" listener right here? Use BrowserApp.deck.addEventListener(.., true) ?

>   addTab: function addTab(aURI, aParams) {
>     aParams = aParams || {};
>-
>     let newTab = new Tab(aURI, aParams);
>     this._tabs.push(newTab);
>+    if (this._doLazyUpdate) {

I don't like _doLazyUpdate. Can't we just do it above?
(Reporter)

Comment 15

4 years ago
Created attachment 8366304 [details] [diff] [review]
Patch v3

Using browser-delayed-startup-finished. Depends on bug 964510.
Attachment #8365282 - Attachment is obsolete: true
Attachment #8366304 - Flags: review?(mark.finkle)
Comment on attachment 8366304 [details] [diff] [review]
Patch v3

I'd rather not move onAppUpdate, but move some of it's contents. Probably making a _delayedStartup method.
Attachment #8366304 - Flags: review?(mark.finkle) → review+
(Reporter)

Comment 18

4 years ago
This was backed out:
76b9e0793b76

I think the errors were likely caused by the ViewFlipper patch. Back in:
https://hg.mozilla.org/integration/fx-team/rev/33060d0581d6
Hi, sorry backedout as part of https://tbpl.mozilla.org/?tree=Fx-Team&rev=c15e29ead092 for the suspicion of causing robocop testfailures on android like https://tbpl.mozilla.org/php/getParsedLog.php?id=33734468&tree=Fx-Team
(Reporter)

Comment 20

4 years ago
Created attachment 8369090 [details] [diff] [review]
Patch 3.1

Same patch, but uses the Java notification to fix some (now broken) tests.
Attachment #8369090 - Flags: review?(mark.finkle)
Attachment #8369090 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/2c2c426e4afe
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
(Reporter)

Comment 23

4 years ago
backed out for startup regressions:
https://hg.mozilla.org/integration/fx-team/rev/72243400ad7c

Updated

4 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
OS: Linux → Android
Hardware: x86 → All
(Reporter)

Comment 24

4 years ago
Comment on attachment 8369090 [details] [diff] [review]
Patch 3.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This caused a startup regression. Requesting uplift of the backout.
User impact if declined: Startup regression
Testing completed (on m-c, etc.): Been on mc for a week.
Risk to taking this patch (and alternatives if risky): low risk
String or IDL/UUID changes made by this patch: none.
Attachment #8369090 - Flags: approval-mozilla-aurora?
Attachment #8369090 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Reporter)

Comment 25

3 years ago
Never got this to land happily. I do see some lazy password manager initialization code in the tree now though (along with the old non-lazy code). Might be worth someone looking into what's going on? Chenxia, do you know anything about password manager startup?
Assignee: wjohnston → nobody
tracking-fennec: --- → ?
Flags: needinfo?(liuche)
I think the real fix for this is Bug 946857.
Component: General → Data Providers
tracking-fennec: ? → +
You need to log in before you can comment on or make changes to this bug.