1005958 - Disable seer until new backend is rewritten

Status: VERIFIED FIXED

(Core :: Networking, enhancement)

Description

[u408661]

Attachment: patch

Now that it's hit release, it turns out the seer causes slow (to non-existent) shutdowns for some users. Let's just disable it on all branches until the new backend is written.

The attached patch is based on m-c, but the same patch works for all branches (including release), perhaps with a bit of fuzz in the line numbers. I already have properly-applied patches for each branch ready.

Comment 1

[Richard Newman [:rnewman]]

Can we also delete netpredictions.sqlite?

Comment 2

[u408661]

Yes, but that's a bigger change, and if we want to get this on release to alleviate the issues there, then let's do just this, and delete the database in a follow-up.

Comment 3

[[:philipp]]

according to feedback from some other users on SUMO (& in my own testing) disabling network.seer.enabled doesn't address the shutdown issue by itself. also see bug 970923

Comment 4

[Patrick McManus [:mcmanus]]

philipp - (In reply to philipp from comment #3)
> according to feedback from some other users on SUMO (& in my own testing)
> disabling network.seer.enabled doesn't address the shutdown issue by itself.
> also see bug 970923

from your experience does this include sessions where it was false at session startup time, or just sessions where you make the change.

(this patch would be the equivalent of making the change before session start)

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 8417443 [details] [diff] [review]
patch

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

at the tuesday meeting lets work out a plan - this is an important part of our story going forward.

::: netwerk/test/unit/xpcshell.ini
@@ -313,5 @@
>  [test_addr_in_use_error.js]
>  [test_about_networking.js]
>  [test_ping_aboutnetworking.js]
>  [test_referrer.js]
>  [test_seer.js]

at least on nightly, let's modifiy the test to turn on the pref itself. Since the code is staying in tree we should keep the tests going. That can be done separately than this bug.

Comment 6

[[:philipp]]

(In reply to Patrick McManus [:mcmanus] from comment #4)
> from your experience does this include sessions where it was false at
> session startup time, or just sessions where you make the change.
> 
> (this patch would be the equivalent of making the change before session
> start)

yes, it's also happening when the setting is false from the beginning. the last thing that shows up in a process monitor log is a file action in relation to netpredictions.sqlite though nevertheless - not sure if this might be related... i will attach a log of such a session.

(for info how i try reproducing the issue see comment 13 at bug 970923).

Comment 7

[[:philipp]]

Attachment: Process Monitor logfile with network.seer.enabled=false

Comment 8

[[:philipp]]

maybe also an interesting observation: when netpredictions.sqlite is deleted before a session starts & network.seer.enabled is set to false, the file will be created in the profile folder during firefox shutdown nevertheless.

Comment 9

[[:philipp]]

...but only when the user has selected to "clear history when firefox closes" (privacy.sanitize.sanitizeOnShutdown=true) and not otherwise.

Comment 10

[u408661]

(In reply to Patrick McManus [:mcmanus] from comment #5)
> Comment on attachment 8417443 [details] [diff] [review]
> patch
> 
> Review of attachment 8417443 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> at the tuesday meeting lets work out a plan - this is an important part of
> our story going forward.

Agreed (however much I would like the plan to be "take this feature out back, shoot it, and dump it in the lake", that's not really a valid option).

> at least on nightly, let's modifiy the test to turn on the pref itself.
> Since the code is staying in tree we should keep the tests going. That can
> be done separately than this bug.

Bug 1006091 (not sure why I didn't do that in the first place... naive optimism, I guess).

Comment 11

[u408661]

Comment on attachment 8417443 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): 881804 (in the beginning)
User impact if declined: Possibility of slow shutdowns (see bug 966469 comment 4)
Testing completed (on m-c, etc.): Well... we've done this before, even though this particular instance hasn't landed on m-c quite yet.
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8417443 [details] [diff] [review]
patch

We are taking this for a non-urgent 29.0.1 release.

Comment 13

[u408661]

remote:   https://hg.mozilla.org/releases/mozilla-release/rev/bebd4af02d88

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1b8d0fcafd1f

Comment 15

[Patrick McManus [:mcmanus]]

nick - what about comments 8 and 9?

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9798ed7822e5
https://hg.mozilla.org/releases/mozilla-beta/rev/ae17ae04609b

Comment 17

[[:philipp]]

the behaviour described in comments 6-9 is unchanged and still happening 29.0.1 candidate build1

Comment 18

[u408661]

Yeah, the behavior in comments 6-9 is annoying, and certainly not optimal, but it's also not saving any browsing data to disk. Basically, when clearing private data, Seer::Reset is called, which makes sure the db exists before deleting everything from the tables. In the case where the db file didn't exist, it gets created with a bunch of empty tables... which then have everything (in this case, nothing) deleted from them.

The behavior may be worth fixing, but only in a strictly train-riding fashion (though I would prefer to focus on rewriting the backend entirely, so that work doesn't get held up by minor issues with a disabled feature).

Comment 19

[[:philipp]]

however, the failure of firefox to shutdown properly is a major inconvenience (though i'm not sure if it's related to this).

Comment 20

[u408661]

(In reply to philipp from comment #19)
> however, the failure of firefox to shutdown properly is a major
> inconvenience (though i'm not sure if it's related to this).

Absolutely, though given that SuMO has reports of flipping the pref fixing the issue for at least some people, I doubt that the behavior described in comment 18 is the cause of it.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/ae17ae04609b

Comment 22

[Petruta Horea [:phorea], Desktop QA]

Unfortunately, we couldn't reproduce any slow shutdowns on Firefox 29.0 (several platforms covered) having the network.seer.enabled true (as default), so we can't confirm that setting this pref to false fixes this problem.

We managed to reproduce it by setting the pref [1] privacy.sanitize.sanitizeOnShutdown to true or by selecting [2] "Clear history when Firefox closes" option in Firefox Preferences. In each case, the STR are the same:
1. Set the above pref (either [1] or [2])
2. Restart browser
3. Load 3-4 full HD videos from Youtube
4. Wait a few minutes
5. Close Firefox

Results: The Firefox process is not closed.
Reproducible also on Firefox 29.0.1.

Those steps (and others) are already logged in bug 1005487 - so maybe this two issues aren't related, but still it would be good to have them both fixed for Fx 29.0.1.

Nicholas, juanb, any thoughts on this? Thank you!

Comment 23

[u408661]

No thoughts on sanitize (quite a bit of that is not in necko, so it's not my area of expertise), except that it's not surprising that using sanitize could cause firefox to not close... that's a lot of data that needs to be deleted synchronously before the process can exit.

Comment 24

[juan becerra [:juanb]]

Is disabling seer related to the issue described in bug 1005487?

We need to know whether the issues described in comment #22 should block our releasing Firefox 29.0.1. Could anyone please evaluate this?

Comment 25

[u408661]

(In reply to juan becerra [:juanb] from comment #24)
> Is disabling seer related to the issue described in bug 1005487?
> 
> We need to know whether the issues described in comment #22 should block our
> releasing Firefox 29.0.1. Could anyone please evaluate this?

I doubt the seer is entirely the cause of bug 1005487, for 2 reasons:

(1) To my knowledge, we only have one report of disabling the seer fixing the issue for someone (see the link in bug 966469 comment 4)
(2) The seer was landed (but pref'd off) in both firefox 27 and firefox 28, without (to my knowledge) these complaints associated with it, so I'm confident that this bug has fixed any seer-related causes of bug 1005487. This leaves other sources as the cause of slow shutdowns.

Comment 26

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to Petruta Rasa [QA] [:petruta] from comment #22)

Do not forget also about this bug #966469

Comment 27

[u279076]

Marking this verified fixed since the intent of this bug has been addressed. Follow-up issues related this should be addressed in separate bugs (ex. bug 966469).

Comment 28

[Nickolay_Ponomarev]

For the reference, the rewrite is bug 1009122.