Closed Bug 1419825 Opened 7 years ago Closed 7 years ago

callers of insertVisitedURIs may overwrite the history title when the input title is null

Categories

(Toolkit :: Places, enhancement, P1)

Product:
Toolkit
Component:
Places
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: mak, Assigned: mak)

References

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

Bug 1419825 - Callers of insertVisitedURIs may overwrite the history title passing a null title.
59 bytes, text/x-review-board-request
lina
: review+
Details 
InsertVisitedURIs calls updatePlaces that sets title to NULL if the passed-in title is empty. This makes so that we may clear the title even if nobody provided a new one. In the case of a normal visit, we'd shortly get a setURITitle call fixing things.
Attachment #8931833 - Flags: review?(kit)
Kit, please check what Sync does regarding this, does it ever pass out an empty string when the title is unknown or should not be changed?
Comment on attachment 8931833 [details]
Bug 1419825 - Callers of insertVisitedURIs may overwrite the history title passing a null title.

https://reviewboard.mozilla.org/r/202954/#review208868

I'm not sure how frequently pages clear their own titles in practice, but this would be a change for how Sync works. Currently, we'll upload `null` if the page has no title, or if it was cleared. Before, that would clear the title when we downloaded and applied the record; with your patch, it'll keep the previous title. So I think we also need to add an `IFNULL` to https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/toolkit/components/places/PlacesSyncUtils.jsm#223, and maybe expand the `fetchURLInfoForGuid` test to check for that. Does that sound right to you?
Attachment #8931833 - Flags: review?(kit)
(In reply to Kit Cambridge (he/him) [:kitcambridge] from comment #3)
> I'm not sure how frequently pages clear their own titles in practice

Probably not often, the only case I may think of is a page that becomes a redirect.

> So I think we also need to add an `IFNULL` to
> https://searchfox.org/mozilla-central/rev/
> 7a8c667bdd2a4a32746c9862356e199627c0896d/toolkit/components/places/
> PlacesSyncUtils.jsm#223, and maybe expand the `fetchURLInfoForGuid` test to
> check for that. Does that sound right to you?

I'll look into that, off-hand it sounds right.
Attachment #8931833 - Flags: review?(kit)
There are a few failures to investigate
the bug was just the mispositioned rv check in History.cpp. The tests refactorings are not  strictly necessary, but while I was there...
Comment on attachment 8931833 [details]
Bug 1419825 - Callers of insertVisitedURIs may overwrite the history title passing a null title.

https://reviewboard.mozilla.org/r/202954/#review210302

Thanks!

::: toolkit/components/places/History.cpp:1020
(Diff revision 3)
>          place.placeId = lastFetchedPlace->placeId;
>          place.guid = lastFetchedPlace->guid;
>          place.lastVisitId = lastFetchedPlace->visitId;
>          place.lastVisitTime = lastFetchedPlace->visitTime;
> +        if (place.title.IsVoid())
> +          place.title = lastFetchedPlace->title;

If we always set `place.title` to the current title here, will we ever hit `titleIsVoid` and skip changing the title in `UpdatePlace`? Can we just drop this entire `if` block?
Attachment #8931833 - Flags: review+
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/c070c95edef7
Callers of insertVisitedURIs may overwrite the history title passing a null title. r=kitcambridge
https://hg.mozilla.org/mozilla-central/rev/c070c95edef7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: