updatePlaces can mistakenly change typed and hidden attributes of a page

RESOLVED FIXED in Firefox 36

Status

()

Toolkit
Places
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

unspecified
mozilla38
Points:
5
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox35 wontfix, firefox36 fixed, firefox37 fixed, firefox38 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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.
Flags: qe-verify+
Flags: in-testsuite?
Flags: firefox-backlog+

Updated

4 years ago
Iteration: --- → 37.3

Updated

4 years ago
Iteration: 37.3 - 12 Jan → 38.1 - 26 Jan
(Assignee)

Comment 1

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

Comment 2

3 years ago
Created attachment 8554174 [details] [diff] [review]
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]);
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+

Updated

3 years ago
Iteration: 38.1 - 26 Jan → 38.2 - 9 Feb
(Assignee)

Comment 4

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/c64734310977
status-firefox35: --- → wontfix
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → affected
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → mozilla38
https://hg.mozilla.org/mozilla-central/rev/c64734310977
Status: ASSIGNED → RESOLVED
Last Resolved: 3 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?
(Assignee)

Comment 8

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

Comment 9

3 years ago
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?
(Assignee)

Updated

3 years ago
status-firefox38: affected → fixed
Attachment #8554174 - Flags: approval-mozilla-beta?
Attachment #8554174 - Flags: approval-mozilla-beta+
Attachment #8554174 - Flags: approval-mozilla-aurora?
Attachment #8554174 - Flags: approval-mozilla-aurora+
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
status-firefox36: fixed → affected
status-firefox37: fixed → affected
Flags: needinfo?(mak77)
(Assignee)

Comment 12

3 years ago
heh, PlacesTestUtils doesn't exist in Aurora and Beta, I'll make a version of the test that doesn't use it.
(Assignee)

Comment 13

3 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/a3559f0f4a13
https://hg.mozilla.org/releases/mozilla-beta/rev/85a7c4ca81f9
status-firefox36: affected → fixed
status-firefox37: affected → fixed
Flags: needinfo?(mak77)
You need to log in before you can comment on or make changes to this bug.