Disable seer until new backend is rewritten

VERIFIED FIXED in Firefox 29

Status

()

enhancement
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: u408661, Assigned: u408661)

Tracking

Trunk
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 verified, firefox30 verified, firefox31 verified, firefox32 verified, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

5 years ago
Posted patch patchSplinter Review
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.
Attachment #8417443 - Flags: review?(mcmanus)
Can we also delete netpredictions.sqlite?
Assignee

Comment 2

5 years ago
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

5 years ago
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
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 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.
Attachment #8417443 - Flags: review?(mcmanus) → review+

Comment 6

5 years ago
(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 8

5 years ago
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

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

Comment 10

5 years ago
(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).
Assignee

Comment 11

5 years ago
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
Attachment #8417443 - Flags: approval-mozilla-release?
Attachment #8417443 - Flags: approval-mozilla-beta?
Attachment #8417443 - Flags: approval-mozilla-aurora?
Comment on attachment 8417443 [details] [diff] [review]
patch

We are taking this for a non-urgent 29.0.1 release.
Attachment #8417443 - Flags: approval-mozilla-release? → approval-mozilla-release+
https://hg.mozilla.org/mozilla-central/rev/1b8d0fcafd1f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Attachment #8417443 - Flags: approval-mozilla-beta?
Attachment #8417443 - Flags: approval-mozilla-beta+
Attachment #8417443 - Flags: approval-mozilla-aurora?
Attachment #8417443 - Flags: approval-mozilla-aurora+
nick - what about comments 8 and 9?

Comment 17

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

Comment 18

5 years ago
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

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

Comment 20

5 years ago
(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.
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!
Flags: needinfo?(hurley)
Assignee

Comment 23

5 years ago
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.
Flags: needinfo?(hurley)
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?
Assignee

Comment 25

5 years ago
(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.
(In reply to Petruta Rasa [QA] [:petruta] from comment #22)

Do not forget also about this bug #966469

Updated

5 years ago
Blocks: 1005566
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).
For the reference, the rewrite is bug 1009122.
See Also: → 1009122
You need to log in before you can comment on or make changes to this bug.