Closed
Bug 1117072
Opened 9 years ago
Closed 9 years ago
updatePlaces can mistakenly change typed and hidden attributes of a page
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
People
(Reporter: mak, Assigned: mak)
References
Details
Attachments
(1 file)
28.61 KB,
patch
|
ttaubert
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•9 years ago
|
Iteration: --- → 37.3
Updated•9 years ago
|
Iteration: 37.3 - 12 Jan → 38.1 - 26 Jan
Assignee | ||
Comment 1•9 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•9 years ago
|
||
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 3•9 years ago
|
||
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•9 years ago
|
Iteration: 38.1 - 26 Jan → 38.2 - 9 Feb
Assignee | ||
Comment 4•9 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
Closed: 9 years ago
Resolution: --- → FIXED
Comment 7•9 years ago
|
||
This seems to be covered to some extent by automated tests. Is manual testing also needed here?
Assignee | ||
Comment 8•9 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•9 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•9 years ago
|
Updated•9 years ago
|
Attachment #8554174 -
Flags: approval-mozilla-beta?
Attachment #8554174 -
Flags: approval-mozilla-beta+
Attachment #8554174 -
Flags: approval-mozilla-aurora?
Attachment #8554174 -
Flags: approval-mozilla-aurora+
Comment 10•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/df4bef8efefd https://hg.mozilla.org/releases/mozilla-beta/rev/adbc58813581
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
Assignee | ||
Comment 12•9 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•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/a3559f0f4a13 https://hg.mozilla.org/releases/mozilla-beta/rev/85a7c4ca81f9
You need to log in
before you can comment on or make changes to this bug.
Description
•