1117072 - updatePlaces can mistakenly change typed and hidden attributes of a page

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

Looks like when updatePlaces is invoked with more than one visit for a single place, we might skip FetchPageInfo here 
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/History.cpp#922

That's an optimization so we don't roundtrip to the db when not needed.

The problem is that internally FetchPageInfo fixes up title, typed and hidden of the newly inserted visit based on the existing page, if we don't do that fixup we might end up doing a wrong UPDATE.

I've not completed investigation yet so this i mostly a guess based on code inspection. First thing will be to write a unit test and check the code paths in the debugger.

Comment 1

[Marco Bonardo [:mak]]

This might even be worse than expected, cause looks like we might mark pages as hidden when they are synced, that means they completely disappear from some views.

Btw, I have a test and an idea to patch it.

Comment 2

[Marco Bonardo [:mak]]

Attachment: patch v1

So the problem here is that FetchPageInfo did some internal transformation on the data fetched from the db, but sometimes we were skipping the db lookup and thus that transformation was not happening.
Since the case where we need that is limited to inserting, I made it do a plain db lookup and moved the transformation outside so that typed = 1 or hidden = 0 always win.

The bug happens when one does (pseudocode):

updatePlaces([{uri1, typed}, {uri1, untyped]);

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8554174 [details] [diff] [review]
patch v1

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

LGTM. Took me a while to understand the logic but I think it's all correct.

Comment 4

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/fx-team/rev/c64734310977

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/c64734310977

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Should we uplift this?

Comment 7

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

This seems to be covered to some extent by automated tests. Is manual testing also needed here?

Comment 8

[Marco Bonardo [:mak]]

I think it might be worth to have a manual test of bug 771278, rather than this one, it's basically the same, but the interesting part is verifying the Sync consequences

Yes, we should uplift as far as possible, now going to ask for approval.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8554174 [details] [diff] [review]
patch v1

Approval Request Comment
[Feature/regressing bug #]: Longstanding regression
[User impact if declined]: When syncing a device (mobile) history, some pages may disappear from the awesomebar
[Describe test coverage new/current, TreeHerder]: automated test
[Risks and why]: low risk based on the fact the patch is contained and tested. A backout is possible without consequences (apart reintroducing this bug).
[String/UUID change made/needed]: none

Comment 10

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/df4bef8efefd
https://hg.mozilla.org/releases/mozilla-beta/rev/adbc58813581

Comment 11

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

Backed out of both aurora and beta for xpcshell bustage:
https://hg.mozilla.org/releases/mozilla-aurora/rev/282e97a8f089
https://hg.mozilla.org/releases/mozilla-beta/rev/992f411762d0

https://treeherder.mozilla.org/logviewer.html#?job_id=548281&repo=mozilla-aurora

Comment 12

[Marco Bonardo [:mak]]

heh, PlacesTestUtils doesn't exist in Aurora and Beta, I'll make a version of the test that doesn't use it.

Comment 13

[Marco Bonardo [:mak]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a3559f0f4a13
https://hg.mozilla.org/releases/mozilla-beta/rev/85a7c4ca81f9