Closed Bug 1433549 Opened 6 years ago Closed 6 years ago

FAIL | toolkit/components/places/tests/unit/test_telemetry.js

Categories

(Thunderbird :: Testing Infrastructure, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 60.0

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

Details

Attachments

(1 file)

FAIL | toolkit/components/places/tests/unit/test_telemetry.js | test_execute
[test_execute : 16] false == true

This appeared today on all platforms.

M-C last good: 7921dcc2bf6bd1ab5ca58e763caefc29ba
M-C first bad: 723b25eb3dd83d0bb1ea846814e9d7e1bd

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7921dcc2bf6bd1ab5ca58e763caefc29ba&tochange=723b25eb3dd83d0bb1ea846814e9d7e1bd

https://public-artifacts.taskcluster.net/Lwetec6NS7yJJTZqMDFZ6g/0/public/logs/live_backing.log says:

INFO -  /builds/worker/workspace/build/tests/xpcshell/tests/toolkit/components/places/tests/unit/test_telemetry.js:PLACES_DATABASE_FAVICONS_FILESIZE_MB:16
INFO -  /builds/worker/workspace/build/tests/xpcshell/tests/toolkit/components/places/tests/unit/test_telemetry.js:test_execute:169

This comes from here:
https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/toolkit/components/places/tests/unit/test_telemetry.js#169

Looks like it was caused be https://hg.mozilla.org/mozilla-central/rev/a39f7d5c0041, bug 1346554.

Marco and Mark, any idea why this now fails in the TB xpcshell test suite?
Flags: needinfo?(standard8)
Flags: needinfo?(mak77)
Best guess without debugging: iirc Thunderbird disables history, if so, there's not going to be anything saved in the pre favicon file, so it isn't going to reduce the size of it.
Flags: needinfo?(standard8)
Thanks for the guess. Does TB really disable history? Doesn't history make links a different colour in messages when you revisit the message where you clicked the link before. Clearing history will restore the previous colour.

Tom said on IRC: Actually, I think it might be https://searchfox.org/mozilla-central/source/toolkit/components/places/PlacesUtils.jsm#969-973 which expects a property to be able to be undefined. ... If it is actually running in strict mode, it will error instead.

I don't understand the comment, so I can take it further based on that.
OK, I debugged this a bit. So something is wrong with the PLACES_DATABASE_FAVICONS_FILESIZE_MB. It already errors out at line 169, which is:
    validate(snapshot.sum);

Dumping out 'validate' I see |val => Assert.ok(val > 0)| and dumping out 'snapshot.sum' I see 0.
I'm pretty sure TB used to disable history, obviously that changed at some point.

My next suggestion would be to try making sure all the places.* prefs are the same, and if that fixes it, work out which one is causing the test to be broken. I think that's most likely here, although I did take a quick look at the prefs that TB seems to set and didn't notice anything obvious.
Thanks Mark.

(In reply to Mark Banner (:standard8) from comment #4)
> I'm pretty sure TB used to disable history, obviously that changed at some
> point.
Exactly, we changed that in 2014 here: https://hg.mozilla.org/comm-central/rev/a005bdfa37f3#l1.18

> My next suggestion would be to try making sure all the places.* prefs are
> the same, and if that fixes it, work out which one is causing the test to be
> broken. I think that's most likely here, although I did take a quick look at
> the prefs that TB seems to set and didn't notice anything obvious.
From here downwards we set a heap of places.* prefs
https://dxr.mozilla.org/comm-central/rev/2b56d89a8792fc1af6ae66be5f38cd9d26ecb7e1/mail/app/profile/all-thunderbird.js#689
No idea why.

I noticed that places.database.cache_to_memory_percentage is obsolete and that FF doesn't set places.database.growthIncrementKiB, so the hard-coded default is used in mozilla/toolkit/components/places/Database.cpp.

Setting the growth increment seems to be wrong given the comment: "With places disabled by default, ...".

Removing both preferences makes the test pass.

I think I can safely land this with "post landing review" as I don't expect much opposition to that fix ;-)

Thanks again Mark for putting us onto the correct track here.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)
Attachment #8946207 - Flags: review?(acelists)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/22741bece2a7
remove obsolete and incorrecly set places preferences. r=aceman
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 60.0
Looks like the problem is that with places.database.growthIncrementKiB the db doesn't grow enough for the test to be effective. We could maybe change the test to set places.database.growthIncrementKiB explicitly.
Thanks Marco, but setting it to 0 was just wrong, don't you agree? A left-over from having the history disabled.
I think the places.database.growthIncrementKiB was set so that the places database ins't initialized to 10MB in size, which would be wasted for the empty database (there should be a bug for this). Can you please check in new profiles if this patch doesn't regress it?
(In reply to Jorg K (GMT+1) from comment #8)
> Thanks Marco, but setting it to 0 was just wrong, don't you agree? A
> left-over from having the history disabled.

So, places.database.growthIncrementKiB controls how much the db grows when it needs to grow. The reason is that growing a database file has a cost, thus it's cheaper to grow it in chunks and then use that space.
Setting it to zero makes it not grow the db in chunks, that may be something you want in constrained situations.
I don't see an advantage in setting it to zero, even in TB.

(In reply to :aceman from comment #9)
> I think the places.database.growthIncrementKiB was set so that the places
> database ins't initialized to 10MB in size, which would be wasted for the
> empty database

Note we changed that to 5MiB some time ago, so it would grow to 5MiB. Provided TB uses history somehow, those could just be reused and it should not be a big deal.

If you really care about not wasting a few MiBs, setting the growth pref in the test is likely the right thing to do.
It is bug 672988.
I think it was said we do not want to record history, so any size of places file is wasted. Maybe there was a bug in the places backend that the initial (whether 5 or 10MB) file is created upon first click on a link, even when later it determines that the history is turned off so nothing will be written to the file.
On a new profile without the preference set to 0, I see a 32KB places.sqlite which grows to 5MB on exit when the shm and wal files are "merged".

With the preference set, the initial 32KB file grows to a bit over 1MB on exit. So personally I wouldn't worry about it. It was more of a worry when hard disks had 80MB, but at 1+TB it doesn't matter, even on a small 60GB SSD.
Comment on attachment 8946207 [details] [diff] [review]
1433549-places.patch

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

Strangely the comments around the removed prefs say we have history disabled, but there is pref("places.history.enabled", true); above and a comment saying we do use history.
Yes, according to https://hg.mozilla.org/comm-central/rev/a005bdfa37f3#l1.18 we enabled history again so then those prefs you just removed should have been removed at that time. So the patch seems to be right and the 5MB file growth is correct and expected. It could only be improved if places backend didn't grow the file even to the 5MB when no history entry was saved (so users never touching any webpage in TB would have the 32Kb file). Only after the first store would the file grow. However, these days it may be rare to never touch the history, it could be even the addon manager downloads webpages or data for AMO servers and that could count as some history entries.
Attachment #8946207 - Flags: review?(acelists) → review+
(In reply to :aceman from comment #13)
> ... However, these days it may be rare to
> never touch the history, ...
Personally, I very much appreciate that history is kept, since I can see which links in messages I have already visited. My workflow would be severely disrupted without the links appearing in the "visited" colour.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: