Bug 876002 (killSyncFormHistory)

Remove nsIFormHistory2 so no synchronous form history code remains

RESOLVED FIXED in Firefox 54

Status

()

Toolkit
Form Manager
RESOLVED FIXED
4 years ago
3 days ago

People

(Reporter: (dormant account), Assigned: Aman Dwivedi, Mentored)

Tracking

(Blocks: 2 bugs, {addon-compat, dev-doc-needed})

unspecified
mozilla54
addon-compat, dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [snappy:p1] p=0 [qa-][good first bug][lang=js])

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

4 years ago
i see removeEntry* and schema upgrade stuff using it in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/nsFormHistory.js and schema upgrade stuff in .jsm

Marco/David, should code switching to the new async-only storage interface to prevent these kinds of omissions?
Async-only storage interface will make sure that you don't have sync code mixed with async (except for closing, see bug 874814), so I guess this would help. Mak knows that codebase better than I do, of course.
There are various reasons that disallow us from doing schema upgrades asynchronously, the main one is that most of the services expose a raw connection, in many cases for testing but in other cases those are exposed to other components or add-ons (unfortunately). Another problem is that most of the code doing those schema migrations has very weak error checking that depends on being synchronous.
So some of the components must be carefully rewritten in better async fashion to avoid that.

Regarding formHistory, looks like mxr is again behind and not updating for whatever reason, the code is much different after bug 566746, the situation should be quite better than reported.
MXR looks to be up to date to me for those files.

nsFormHistory.js is clearly still be using main thread/synchronous SQLite. But AIUI that's the "old" back-end that desktop isn't be using anymore (in favor of FormHistory.jsm). The only other commitTransaction call is in the db migration in FormHistory.jsm, and that one really shouldn't be showing up frequently.

nsIFormHistory2 still has consumers in metro, android and sync, though, and I also see a user in SafariProfileMigrator.js and in one test (browser_sanitizeDialog_treeView.js). Seems to me we need to at least get bugs on file to remove all of these uses and actually get rid of nsIFormHistory2.

I wonder whether the sync consumer (services/sync/modules/engines/forms.js) is responsible for the remaining slowsql issues...

(Also, do we need a bug on file to have FormHistory.jsm use SQLite.jsm instead of rolling its own?)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #3)
> The only other commitTransaction call is in the
> db migration in FormHistory.jsm, and that one really shouldn't be showing up
> frequently.

...but we should still get rid of it. As far as I can tell there isn't much reason why FormHistory.jsm's DB migration couldn't be async.
Mark, can you chase this down further (comment 3/comment 4)?

Maybe this is moot if we can just remove all consumers and get rid of the old component entirely, but as it is these two components (nsFormHistory.js and FormHistory.jsm) seem to operate on the same database (formhistory.sqlite), and I wonder if that's going to cause any issues. E.g. there seems to be "migration" code in both of them...
Assignee: nobody → mhammond
As best as Gavin and I can tell, the slowsql dashboard is showing the number of "commit transaction" instances declining - and looking at some of the other related SQL statements, we believe there are still some older nightlies out there which haven't updated since bug 566746 landed.  Once this stabilizes we should find the remaining cases are in other modules which continue to use nsIFormHistory2 - metro and sync.

So we'll treat this as a meta bug to remove nsIFormHistory2 and its implementation in nsFormHistory.js, which should kill those remaining cases.  In the meantime, we can continue to monitor that dashboard to verify our assertions above as more nightlies update themself - at which point we can re-evaluate if this bug should keep its [snappy:p1] status.
OS: Windows 8 → All
Hardware: x86_64 → All
Summary: COMMIT TRANSACTION still top cause of main thread sql in slowsql dash → [meta] Remove nsIFormHistory2 so no synchronous form history code remains

Updated

4 years ago
Depends on: 878677

Updated

4 years ago
Depends on: 878690

Updated

4 years ago
Depends on: 878691

Updated

4 years ago
Depends on: 878692

Updated

4 years ago
Depends on: 879118

Updated

4 years ago
Blocks: 699820
(Reporter)

Comment 7

4 years ago
Created attachment 757982 [details]
total number of sessions with fomrhistory slowsql
(Reporter)

Comment 8

4 years ago
Created attachment 757983 [details]
number of slow sql queries(eg multiple queries per session)
(Reporter)

Comment 9

4 years ago
Created attachment 757984 [details]
total number of sessions with formhistory slowsql
Attachment #757982 - Attachment is obsolete: true
(Reporter)

Comment 10

4 years ago
jydoop script used to generate the data: https://github.com/mozilla/jydoop/blob/master/scripts/slowsql.py

ran it with time make ARGS="scripts/slowsql.py outputfile 20130512 20130603" hadoop
Keywords: addon-compat

Updated

4 years ago
Alias: killSyncFormHistory

Updated

4 years ago
Depends on: 886820
No longer depends on: 886820
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla24 → ---
The synchronous API should have no internal consumers in our codebase (apart dedicated tests) and warn deprecation to consumers at this point. If you can find anything else that is not a test, please file a dependency.

This bug will take care of finally removing the API and all remaining consumers, once we are comfortable with the removal, usually we wait at least a couple versions with the deprecation.
Data from bug 878677 comment 37 confirms that we've eliminated the majority of the form history main thread queries - good work everyone :)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #12)
> Data from bug 878677 comment 37 confirms that we've eliminated the majority
> of the form history main thread queries - good work everyone :)

Note to myself since it's not obvious from bug metadata - this happened in the Firefox 24 cycle.

Updated

3 years ago
Blocks: 987745
this is actionable.
Blocks: 981530
No longer blocks: 987745

Updated

3 years ago
Blocks: 987745

Updated

3 years ago
Blocks: 950073
No longer blocks: 981530
Whiteboard: [snappy:p1] → [snappy:p1] p=0

Updated

3 years ago
No longer blocks: 950073
Flags: firefox-backlog+
Summary: [meta] Remove nsIFormHistory2 so no synchronous form history code remains → Breakdown: Remove nsIFormHistory2 so no synchronous form history code remains
Whiteboard: [snappy:p1] p=0 → [snappy:p1] p=0 [qa-]
Mark, are you actively working on this, or would you like to mentor the remaining work here?
Flags: needinfo?(mhammond)
Not working on it and very happy to mentor!
Assignee: mhammond → nobody
Status: REOPENED → NEW
Flags: needinfo?(mhammond)
Cool, when you have the time, please post a comment or dependent bugs with leftover work to do.
So far I just did a quick search
http://mxr.mozilla.org/mozilla-central/search?string=nsIFormHistory2
Mentor: mhammond@mozilla.com
Whiteboard: [snappy:p1] p=0 [qa-] → [snappy:p1] p=0 [qa-][good next bug][lang=js]
https://dxr.mozilla.org/mozilla-central/search?q=nsIFormHistory2&redirect=false
Whiteboard: [snappy:p1] p=0 [qa-][good next bug][lang=js] → [snappy:p1] p=0 [qa-][good first bug][lang=js]
(Assignee)

Comment 19

4 months ago
Hi Mark! I would like to take this up. Can you please help me with this? :)
Flags: needinfo?(markh)
(In reply to Aman Dwivedi from comment #19)
> Hi Mark! I would like to take this up. Can you please help me with this? :)

Sorry for the delay and I'm really glad you want to help :)

If you look at the search link in comment 18, you will see there are a small number of references to nsIFormHistory2 left in the tree. Specifically:

* nsIFormHistory.idl can be removed - a reference to it will also need to be removed from toolkit/components/satchel/moz.build.

* The references to nsIFormHistory.h can be ignored - removing the IDL will take care of that.

* services/common/tests/unit/test_async_querySpinningly.js can also be removed - it's an old test that isn't relevant now that sync itself no longer uses this interface. There will be a reference to this file in xpcshell.ini that will also need to be removed.

* There is a reference in a comment in toolkit/components/satchel/FormHistory.jsm which can be left alone.

And as far as I can tell, that's it (although obviously we will be looking for a try run once the patch is up, but we can help you out with that.

Mak, does that sound right to you, or is there something I'm missing?
Flags: needinfo?(markh) → needinfo?(mak77)
(In reply to Mark Hammond [:markh] from comment #20)
> * services/common/tests/unit/test_async_querySpinningly.js can also be
> removed - it's an old test that isn't relevant now that sync itself no
> longer uses this interface. There will be a reference to this file in
> xpcshell.ini that will also need to be removed.

To me looks like that method is still used in history and bookmarks engines:
http://searchfox.org/mozilla-central/search?q=querySpinningly&path=
And I don't see another test checking that querySpinningly works fine.
Maybe the test should be modified and simplified to use a memory database instead of form history?

> * There is a reference in a comment in
> toolkit/components/satchel/FormHistory.jsm which can be left alone.

I'd modify the comment to just state "// We don't support any schema version before FormHistory.jsm."

And nsFormHistory.js should also be removed, since it's the implementation of nsIFormHistory2. Thus also these references should be removed:
http://searchfox.org/mozilla-central/search?q=nsFormHistory.js
Flags: needinfo?(mak77)
Created attachment 8826998 [details] [diff] [review]
querySpinningly-test.patch

Thanks Mak,

(In reply to Marco Bonardo [::mak] from comment #21)
> (In reply to Mark Hammond [:markh] from comment #20)
> > * services/common/tests/unit/test_async_querySpinningly.js can also be
> > removed - it's an old test that isn't relevant now that sync itself no
> > longer uses this interface. There will be a reference to this file in
> > xpcshell.ini that will also need to be removed.
> 
> To me looks like that method is still used in history and bookmarks engines:
> http://searchfox.org/mozilla-central/search?q=querySpinningly&path=
> And I don't see another test checking that querySpinningly works fine.

Yeah, I think that's probably OK, but not ideal.

> Maybe the test should be modified and simplified to use a memory database
> instead of form history?

In this patch, I just did a few hacks allowing the test to still use the form history DB - it's a bit hacky, but IMO it is OK and exercising what it is trying to exercise. I expect querySpinningly to be dead later this year.

Aman, if you apply this patch to that problematic test and perform all the other steps described, I'd be happy to review it.
(Assignee)

Comment 23

3 months ago
Created attachment 8830658 [details] [diff] [review]
BugFix876002.patch

Hi Mark! Sorry for delay. Please review the patch and leave your comments. Thanks!
Attachment #8830658 - Flags: review?(markh)
Comment on attachment 8830658 [details] [diff] [review]
BugFix876002.patch

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

Thanks for the patch! It looks good, but there are a few changes to make.

See also comment 21 where Mak suggests a change to FormHistory.jsm

::: services/common/tests/unit/test_async_querySpinningly.js
@@ -1,1 @@
> -/* Any copyright is dedicated to the Public Domain.

Please see comment 22 - the patch there should be used instead of removing this file.
Attachment #8830658 - Flags: review?(markh)
when you remove formHistory.js, you should also remove all the references to it:
http://searchfox.org/mozilla-central/search?q=nsFormHistory.js
(Assignee)

Comment 26

3 months ago
Created attachment 8833723 [details] [diff] [review]
BugFix876002.patch

Sorry for delay! This patch should work. :)
Attachment #8830658 - Attachment is obsolete: true
Attachment #8833723 - Flags: review?(markh)
Comment on attachment 8833723 [details] [diff] [review]
BugFix876002.patch

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

This is looking good, but there are still a few things that need to be corrected. After you post a new version I'll push it to try and we can move towards getting it checked in - thanks!

::: services/common/tests/unit/test_async_querySpinningly.js
@@ +11,4 @@
>  
>  const SQLITE_CONSTRAINT_VIOLATION = 19;  // http://www.sqlite.org/c3ref/c_abort.html
>  
> +// This test is a buit hacky - it was originally written to use the

oops - I know I supplied this change, but I mis-spelled "but" as "buit"

::: services/common/tests/unit/xpcshell.ini
@@ -32,4 @@
>  [test_utils_uuid.js]
>  
>  [test_async_chain.js]
> -[test_async_querySpinningly.js]

This line shouldn't be removed as the test file hasn't been removed.

::: toolkit/components/satchel/satchel.manifest
@@ +1,1 @@
>  contract @mozilla.org/satchel/form-history;1 {0c1bb408-71a2-403f-854a-3a0659829ded}

this line should be removed too - it is part of the nsFormHistory.js component definition.
Attachment #8833723 - Flags: review?(markh)
(Assignee)

Comment 28

3 months ago
Created attachment 8833951 [details] [diff] [review]
BugFix876002.patch

Thanks Mark! Have made the changes. Please review! :)
Attachment #8833723 - Attachment is obsolete: true
Attachment #8833951 - Flags: review?(markh)
Comment on attachment 8833951 [details] [diff] [review]
BugFix876002.patch

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

LGTM, thanks! I've pushed a try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=c44710a07f1856c2ee7eccc6565ae215adc7a6f7, and if that looks green we can get this pushed \o/

Updated

3 months ago
Assignee: nobody → dwivedi.aman96

Updated

3 months ago
Attachment #8826998 - Attachment is obsolete: true
Created attachment 8835316 [details] [diff] [review]
0001-Bug-876002-Remove-nsIFormHistory2-so-no-synchronous-.patch

Thanks! Sadly the try-push failed in debug builds due to my changes to test_async_querySpinningly.js, so I just made a test-only change to your patch which closes the database at the end of the test.

Thanks for your contribution - we should find it gets pushed in the next 24 hours.
Attachment #8833951 - Attachment is obsolete: true
Attachment #8833951 - Flags: review?(markh)
Attachment #8835316 - Flags: review+

Updated

3 months ago
Keywords: checkin-needed

Comment 31

3 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5fb59f54b4b
Remove nsIFormHistory2 so no synchronous form history code remains, r=markh.
Keywords: checkin-needed

Updated

3 months ago
Summary: Breakdown: Remove nsIFormHistory2 so no synchronous form history code remains → Remove nsIFormHistory2 so no synchronous form history code remains

Comment 32

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c5fb59f54b4b
Status: NEW → RESOLVED
Last Resolved: 4 years ago3 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Comment 33

7 days ago
This change has broken some addons. Is there an alternative of nsIFormHistory and nsIFormHistory2? And a documentation available?

Comment 34

6 days ago
https://developer.mozilla.org/en-US/Firefox/Releases/54#Changes_for_add-on_and_Mozilla_developers
Keywords: dev-doc-needed

Comment 35

6 days ago
(In reply to Danny Lin from comment #33)
> This change has broken some addons. Is there an alternative of
> nsIFormHistory and nsIFormHistory2? And a documentation available?

May be https://github.com/mozilla/gecko-dev/blob/master/toolkit/components/satchel/FormHistory.jsm
also, this was logging a deprecated warning to the console from a lot of versions (4 years!), those add-ons are probably unmaintained if the developer didn't notice the warnings.
You need to log in before you can comment on or make changes to this bug.