Remove "FormHistory:Init" and "Passwords:Init" messages

RESOLVED WONTFIX

Status

()

Firefox for Android
Data Providers
RESOLVED WONTFIX
5 years ago
2 years ago

People

(Reporter: Margaret, Unassigned, Mentored)

Tracking

Trunk
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=js][lang=java])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Follow-up to bug 878692.

It looks like these DBs will always get created once Gecko is up and running, so we don't need to force it with these init messages.
(Reporter)

Comment 1

5 years ago
Created attachment 757744 [details] [diff] [review]
WIP

Here's a quick WIP. I'm wondering if we still need to be doing something in PerProfileContentProvider to initalize Gecko when we find that the DBs don't exist.

It seems fishy that we're sending "Passwords:Init" in PasswordsProvider, but also sending it in GeckoMessageReceiver. Also, why are we only using that for passwords, but not for form history?
Attachment #757744 - Flags: feedback?(wjohnston)
Attachment #757744 - Flags: feedback?(rnewman)
Comment on attachment 757744 [details] [diff] [review]
WIP

Review of attachment 757744 [details] [diff] [review]:
-----------------------------------------------------------------

I agree, I worry a little too. I guess the downside is, if your db is somehow erased, but your profile isn't, we wouldn't recreate it until you did something that created it, and syncs would fail. If you've lost your databases, but not your profile, something really strange is up, but I'm hoping rnewman knows better about it.

The passwords stuff is different because it runs in its own process and uses intents to communicate with the main one. This is probably the simplest way to do that.
Attachment #757744 - Flags: feedback?(wjohnston) → feedback+
Comment on attachment 757744 [details] [diff] [review]
WIP

Review of attachment 757744 [details] [diff] [review]:
-----------------------------------------------------------------

As Wes mentioned, that chunk of passwords code is there to make sure pwmgr is running in its separate process when the password provider is (though I don't see how it will keep it alive, mm? Does it ever die?).

My concern here is that there's nothing connecting FormHistoryProvider to Gecko, unless you fill in that "XXX". If a sync tries to run but Fennec isn't in the background, the code you added to browser.js won't run.

So in theory:

* Upgrade Fennec such that a form history DB schema change happens.
* On another device, add form history items and sync.
* Accounts > Firefox Sync > Sync Now.

The sync should fail, or cause damage to the DB (because the schema upgrade didn't occur -- you're cross-mojinating SQLiteOpenHelper and mozStorage upgrades).

Probably that "XXX" would just be a lifting of the sendEventToGecko that you're deleting from FormHistoryProvider, no? And in that case, maybe this patch will shrink a lot.

So, in the spirit of Socratic reviewing: can you prove to me that accessing the FHP without first launching GeckoApp will cause the DB to be correctly initialized and upgraded?

(And, in the spirit of fairness, I don't know if it *currently* does so.)

If you can prove that there's no regression, then rock on.
Attachment #757744 - Flags: feedback?(rnewman) → feedback+
I don't think it happens. If you try to access form history (or passwords) and Gecko has never run (or the db needs an upgrade), the access will fail (since we can't throw across process, you just get a return value that indicates no insert/update/delete/etc. happened).
(Reporter)

Comment 5

4 years ago
I haven't had time to work on this.
Assignee: margaret.leibovic → nobody
Whiteboard: [mentor=wesj][lang=js][lang=java]
(Assignee)

Updated

4 years ago
Mentor: wjohnston
Whiteboard: [mentor=wesj][lang=js][lang=java] → [lang=js][lang=java]
Currently we spam

07-16 16:00:01.397 E/GeckoConsole(29384): [JavaScript Error: "storage.init is not a function" {file: "chrome://browser/content/browser.js" line: 1624}]

during startup.

From:

      case "Passwords:Init": {
        let storage = Cc["@mozilla.org/login-manager/storage/mozStorage;1"].
                      getService(Ci.nsILoginManagerStorage);
        storage.init();
        Services.obs.removeObserver(this, "Passwords:Init");
        break;
      }
Blocks: 959776
Hardware: ARM → All
(Reporter)

Comment 7

2 years ago
I don't think it's worth the time and energy to try to do this.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.