Closed Bug 1117072 Opened 5 years ago Closed 5 years ago

updatePlaces can mistakenly change typed and hidden attributes of a page


(Toolkit :: Places, defect)

Not set



38.2 - 9 Feb
Tracking Status
firefox35 --- wontfix
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed


(Reporter: mak, Assigned: mak)




(1 file)

Looks like when updatePlaces is invoked with more than one visit for a single place, we might skip FetchPageInfo here

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.
Flags: qe-verify+
Flags: in-testsuite?
Flags: firefox-backlog+
Iteration: --- → 37.3
Iteration: 37.3 - 12 Jan → 38.1 - 26 Jan
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.
Attached patch patch v1Splinter Review
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]);
Attachment #8554174 - Flags: review?(ttaubert)
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.
Attachment #8554174 - Flags: review?(ttaubert) → review+
Iteration: 38.1 - 26 Jan → 38.2 - 9 Feb
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → mozilla38
Closed: 5 years ago
Resolution: --- → FIXED
Should we uplift this?
Flags: needinfo?(mak77)
This seems to be covered to some extent by automated tests. Is manual testing also needed here?
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.
Flags: qe-verify-
Flags: qe-verify+
Flags: needinfo?(mak77)
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
Attachment #8554174 - Flags: approval-mozilla-beta?
Attachment #8554174 - Flags: approval-mozilla-aurora?
Attachment #8554174 - Flags: approval-mozilla-beta?
Attachment #8554174 - Flags: approval-mozilla-beta+
Attachment #8554174 - Flags: approval-mozilla-aurora?
Attachment #8554174 - Flags: approval-mozilla-aurora+
heh, PlacesTestUtils doesn't exist in Aurora and Beta, I'll make a version of the test that doesn't use it.
You need to log in before you can comment on or make changes to this bug.