449406 - bookmarks of TRANSITION_DOWNLOAD and TRANSITION_EMBED don't show up in the location bar if they've been visited

Status: VERIFIED FIXED

(Toolkit :: Places, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: Current trunk

STEPS TO REPRODUCE:
1) Bookmarks > Organize Bookmarks
2) Bring up the context menu on "Unsorted Bookmarks" in the pane on the left
3) Select "New Bookmark..."
4) Enter "Phishing slides" as the name
5) Enter "http://www.cs.auckland.ac.nz/~pgut001/pubs/phishing.pdf" as the
   location
6) Select "Add"
7) Close bookmark organizer
8) Type "phish" in the URL bar

EXPECTED RESULTS: match list includes the bookmark you added, since both the
                  URL and the name match "phish". 

ACTUAL RESULTS: match list doesn't include that bookmark (in fact, is empty in
                my case)

I don't know whether this is due to me manually adding the bookmark, due to the page not being in history (which it can never be anyway), or something else.

Comment 1

[Phil Ringnalda (:philor)]

WFM, so I suspect "something else."

Comment 2

[Shawn Wilsher :sdwilsh]

works for me as well.  did you change any of your url prefs?

Comment 3

[Boris Zbarsky [:bzbarsky]]

I have no idea.  If it's not exposed in the prefs dialog I probably didn't change it, but I'd be happy to give you any info you need (values for specific pref names, output of stuff run in JS console, etc).

Comment 4

[Ed Lee :Mardak]

Anything changed under the "browser.urlbar." prefbranch?

Comment 5

[Ed Lee :Mardak]

Oh and "places.frecency." too.

Comment 6

[Boris Zbarsky [:bzbarsky]]

No non-default settings under either of those.

Comment 7

[Uberadri]

I have this problem too. About half of the search terms that gives results in the bookmarks pane, does not give any results in the location bar. And very often i get more results for the same search term in the bookmarks pane than in the location bar. I can't see any logic behind which terms/bookmarks are working or not.

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #0)
> 5) Enter "http://www.cs.auckland.ac.nz/~pgut001/pubs/phishing.pdf" as the
>    location
Do you, by chance, have this bookmarked elsewhere too?

Comment 9

[Boris Zbarsky [:bzbarsky]]

No.

Though now that you raise that, I tried adding it a second time and ran into bug 471352 again.

But I continue to have this bookmark in my "unsorted bookmarks", and neither "phishing" nor "slides" nor "phishing slides" nor "auckland" finds it.  "auckland" finds other pages on the same site that I visited, but NOT this one page.

I wonder... did you guys try clicking on the link before trying to reproduce?  And make sure that you don't have a PDF plug-in installed?

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

the entry appear when you add it to bookmarks, but as soon as you visit it its frecency is set to 0, and it won't appear in the locationbar.

try these STRs:
1 - context menu on bookmarks toolbar
2 - New Bookmark...
3 - Enter "Phishing slides" as the name
4 - Enter "http://www.cs.auckland.ac.nz/~pgut001/pubs/phishing.pdf" as location
6 - Select "Add"
7 - Type "phish" in the URL bar
WORKS (frecency = -1)
8 - now click on the bookmark and Cancel the download dialog
9 - Type "phish" in the URL bar
DOES NOT WORK (frecency = 0)
10 - clear history
WORKS (frecency = -1)

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

bug 412219 is related since the problem is in visit_count used in calculateFrecencyInternal. Download and embed do not add to visit_count and have bonus 0, so their calculated frecency is 0 (visit_count = 0 and bonus = 0).
Plus when calling UpdateFrecency we pass aIsBookmark = PR_FALSE even if it's true.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Marco, would I have gotten the same results if I'd visited the PDF before adding the bookmark?  I'm certain I'd done that!

Requesting blocking, though I'm not quite sure this really should block.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Adding a bookmark updates the frecency, but the fact a place is bookmarked is not used to calculate frecency if the place has visits. Indeed the aIsBookmarked param is only used if we don't have visits, if we have visits we exit after sampling a certain number of them, without considering if it is bookmarked. So i suppose the answer to comment 12 is yes, because even bookmarking the site later won't change the frecency from 0.

ideally i think not showing downloads in the locationbar is fine, unless they are not bookmarks. Bookmarked downloads should be visible instead.

The biggest problem about that is that everytime we have to check if an uri is bookmarked we need to filter out livemarks children and that can slowdown us quite a bit (since we also update frecency on add visit).
The clean fix would be to remove livemarks children from bookmarks table (Bug 453530 surely not possible for 3.1), pass a correct isBookmarked param to UpdateFrecency (we only should exclude place: queries then) and in case we have a 0 sample from visits but the uri is bookmarked use unvisited bookmarks bonus.

Comment 14

[Boris Zbarsky [:bzbarsky]]

I guess my issue is that I thought starring a page was supposed to make it show up higher in the awesomebar results, and that in fact that was the whole point of the new bookmark setup.  The first time I actually tried to make use of it, it failed (this bug).

Comment 15

[Ed Lee :Mardak]

Is this just an issue of a bookmark not getting a big enough of a bonus?

There's currently two bookmark related prefs:

places.frecency.bookmarkVisitBonus
places.frecency.unvisitedBookmarkBonus

But those don't give bonuses for just being a bookmark. The former only triggers if you click the bookmark from the bookmark menu (IIRC) and the latter is only for places that are bookmarked but no visits.

One way is to tweak bookmarkVisitBonus to give a bonus to places that are visited and happen to be bookmarks. Similar to places.frecency.typedVisitBonus.

Comment 16

[Ed Lee :Mardak]

Attachment: idea v1

One idea: After checking the visit type for a bonus, additionally check if it's a bookmark and give a minimum of the bookmark visit amount.

Comment 17

[Shawn Wilsher :sdwilsh]

I think it has to do with how we handle TRANSITION_DOWNLOAD actually.

Comment 18

[Ed Lee :Mardak]

Attachment: idea v1.1

A quick refactoring of some "is bookmark" code and this seems to fix the problem of visits of only transition download preventing a page from showing up. Tests are probably broken.

1) I visited the pdf link and nothing shows up when I type "phish".
2) I bookmark "phishing" and it shows up when I type "phish".
3) I change the bookmark url to .blah and .blah shows up but not .pdf.
4) Switch back to Firefox 3, I change bookmark url from .blah to .pdf and no .pdf, but there is a .blah. (Separate bug where we dont recalc frecency on changing uri.. but might already be fixed for 3.1?)

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

see comment 13.
that's bad for performance since we do such a check on every visit, instead we should really have a precompiled statement that will exactly check if an uri is bookmarked, is not a livemark and is not a query, at least until we will remove livemarks children from bookmarks table, then we could directly use isBookmarked and isQuery.

Then i think is not correct to use mBookmarkVisitBonus, because clearing history and setting unvisitedBookmarksBonus to 0 will still show this even if it's an unvisited bookmark.

Plus we call updateFrecency in other places, especially in nsNavBookmarksService, with the wrong value, so we need a precompiled statement there too for now, and we have to fix all calls or some other action could again reset the frecency to 0 and cause an intermittent view/hide behavior.

Comment 20

[Shawn Wilsher :sdwilsh]

Writing this down for my own sanity:
After auditing every call site of UpdateFrecency, the only call site where it is not called with the right value of isBookmarked is in nsNavHistory::AddVisit.

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

comment 20 is wrong, for example see bug 455315, probably the same in every place where we remove a bookmark but we don't check if the uri has more than one bookmark (removeFolderChildren?).
Also updateFrecency is an entering point for CalculateFrecencyInternal that has the bug, we should also check other CalculateFrecencyInternal callers.

Comment 22

[Shawn Wilsher :sdwilsh]

Attachment: v2.0

This is a lot simpler than the previous ideas, and changes things up a bit (and isn't fully correct given comment 21)

Note that this changes behavior -CanAddURI now returns false for query uri's (place:).  I don't see why we would ever want to add a visit for a place uri, and it simplifies some of our logic.

This only ends up doing one query to the database during AddVisit, and uses the bookmark hash table otherwise.

Comment 23

[Shawn Wilsher :sdwilsh]

Marco pointed out on irc that this patch isn't right either since this could be the child of a livemark.  I think it might be worthwhile to not even add livemark children to the bookmarks hash, but I need to investigate that more still too.

Also, the next patch needs to address the issue of duplicate bookmarks.

Comment 24

[Shawn Wilsher :sdwilsh]

So, if we don't add livemark children to the bookmarks hash, we simplify a number of codepaths.  There is a side effect here though - IsBookmarked will return false for livemark children now.  Dietrich, do you think this is problematic?

Comment 25

[Shawn Wilsher :sdwilsh]

Attachment: v3.0

Not there yet, but this builds on the work from bug 412132.  I need to make sure I've addressed previous comments, and write some unit tests still.

Comment 26

[Shawn Wilsher :sdwilsh]

Things I need to write a test for:
(1) autocomplete returns a download and embed uri that is bookmarked and visited
(2) visits are not added for uris for place:

If anyone can think of something else, please let me know.

(In reply to comment #21)
> Also updateFrecency is an entering point for CalculateFrecencyInternal that has
> the bug, we should also check other CalculateFrecencyInternal callers.
There are two call sites:
(1) UpdateFrecency
(2) CalculateFrecency

CalculateFrecency does a proper bookmark check (which I think we actually want to use our method now and a new patch will have that).

Comment 27

[Shawn Wilsher :sdwilsh]

(In reply to comment #26)
> CalculateFrecency does a proper bookmark check (which I think we actually want
> to use our method now and a new patch will have that).
Additional benefit is that we can get rid of mDBGetBookmarkParentsForPlace on nsNavHistory, which will offset the cost of us adding the new statement in nsNavBookmarks.  This may actually speed up frecency calculations.

Comment 28

[Shawn Wilsher :sdwilsh]

Attachment: v3.1

Not sure if I can get the tests done today, so let's get this code looked at...

Comment 29

[Dietrich Ayala (:dietrich)]

Comment on attachment 358504 [details] [diff] [review]
v3.1

>diff --git a/toolkit/components/places/src/nsNavBookmarks.cpp b/toolkit/components/places/src/nsNavBookmarks.cpp
>--- a/toolkit/components/places/src/nsNavBookmarks.cpp
>+++ b/toolkit/components/places/src/nsNavBookmarks.cpp
>@@ -360,16 +360,32 @@ nsNavBookmarks::InitStatements()
>   // mDBIsBookmarkedInDatabase
>   // Just select position since it's just an int32 and may be faster.
>   // We don't actually care about the data, just whether there is any.
>   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>       "SELECT position FROM moz_bookmarks WHERE fk = ?1 AND type = ?2"),
>     getter_AddRefs(mDBIsBookmarkedInDatabase));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  // mDBIsRealBookmark
>+  // Checks to make sure a place_id is a bookmark, and isn't a livemark.
>+  rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>+      "SELECT id "
>+      "FROM moz_bookmarks "
>+      "WHERE fk = ?1 AND "
>+            "type = ?2 AND "
>+            "parent NOT IN ("
>+              "SELECT a.item_id "
>+              "FROM moz_items_annos a "
>+              "JOIN moz_anno_attributes n ON a.anno_attribute_id = n.id "
>+              "WHERE n.name = ?3"
>+            ")"),
>+    getter_AddRefs(mDBIsRealBookmark));
>+  NS_ENSURE_SUCCESS(rv, rv);

add " LIMIT 1"?

>+PRBool
>+nsNavBookmarks::IsRealBookmark(PRInt64 aPlaceId)
>+{
>+  // Fast path is to check the hash table first.  If it is in the hash table,
>+  // then verify that it is a real bookmark.
>+  PRInt64 bookmarkId;
>+  PRBool isBookmark = mBookmarksHash.Get(aPlaceId, &bookmarkId);
>+  if (!isBookmark)
>+    return PR_FALSE;
>+
>+  {
>+    mozStorageStatementScoper scope(mDBIsRealBookmark);
>+
>+    (void)mDBIsRealBookmark->BindInt64Parameter(0, aPlaceId);
>+    (void)mDBIsRealBookmark->BindInt32Parameter(1, TYPE_BOOKMARK);
>+    (void)mDBIsRealBookmark->BindUTF8StringParameter(
>+      2, NS_LITERAL_CSTRING(LMANNO_FEEDURI)
>+    );

nit: check rv. probably paranoid, but is consistent w/ the rest of Places.

>@@ -2755,17 +2749,19 @@ nsNavHistory::AddVisit(nsIURI* aURI, PRT
> 
>   rv = InternalAddVisit(pageID, referringVisitID, aSessionID, aTime,
>                         aTransitionType, aVisitID);
>   transaction.Commit();
> 
>   // Update frecency (*after* the visit info is in the db)
>   // Swallow errors here, since if we've gotten this far, it's more
>   // important to notify the observers below.
>-  (void)UpdateFrecency(pageID, PR_FALSE);
>+  // We cannot get any query URI's here, because CanAddURI would say no.
>+  nsNavBookmarks *bs = nsNavBookmarks::GetBookmarksService();
>+  (void)UpdateFrecency(pageID, bs->IsRealBookmark(pageID));

nit: the comment is not clear. what are you saying, and why there?

patch is looking ok so far.

Comment 30

[Shawn Wilsher :sdwilsh]

Comment on attachment 358504 [details] [diff] [review]
v3.1

something is still wrong with this.  Not ready yet...

Comment 31

[Shawn Wilsher :sdwilsh]

In addition to what I've already attached, we need to modify how we calculate frecency too.  I can get my testcase to pass if I make the bonus for TRANSITION_EMBED be:
bonus = aIsBookmarked ? mBookmarkVisitBonus : mEmbedVisitBonus;
and TRANSITION_DOWNLOAD be:
bonus = aIsBookmarked ? mBookmarkVisitBonus : mDownloadVisitBonus;

The problem is that I'm not so sure that's the right thing to do here.  Thoughts?

Comment 32

[Dietrich Ayala (:dietrich)]

[11:58] <dietrich> sdwilsh: yes. if the url is bookmarked, it should get the bookmark bonus, and we should ignore it's download/embeddedness

Comment 33

[Shawn Wilsher :sdwilsh]

(In reply to comment #29)
> nit: check rv. probably paranoid, but is consistent w/ the rest of Places.
The only reason why I didn't is because this function returns a boolean, so propagating error codes doesn't make sense.  If the binding fails, we'll get nothing returned, so we go to the false case anyway.

Comment 34

[Shawn Wilsher :sdwilsh]

(In reply to comment #26)
> Things I need to write a test for:
> (1) autocomplete returns a download and embed uri that is bookmarked and
> visited
> (2) visits are not added for uris for place:
(2) is no longer valid - don't want to block place: uri's from being added to history.

Comment 35

[Shawn Wilsher :sdwilsh]

Attachment: v3.2

Passes all unit tests, and adds a test to ensure that we don't regress this.

Comment 36

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 358922 [details] [diff] [review]
v3.2

looks good and i like the approach.
I think you should also provide a test to check that if those pages are not bookmarked but visited (embed download) they don't appear, and that if they are bookmarked without visits they appear (unless we already have one test covering that).

Comment 37

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

this is not going to fix other calls to updateFrecency in nsNavBookmarks isn't it? I mean bug 455315, those fixes should be quite easy now, so maybe you could add them to avoid really minor cases in which the frecency could still be zeroed(). Do you want us to fix that apart to provide a specific test?

Comment 38

[Ed Lee :Mardak]

Comment on attachment 358922 [details] [diff] [review]
v3.2

>+++ b/toolkit/components/places/src/nsNavHistory.cpp
>@@ -7084,16 +7079,26 @@ nsNavHistory::CalculateFrecencyInternal(
>+      // If this is a bookmark, regardless of the transition type, we always
>+      // need to add the bookmark visit bonus.
>+      if (aIsBookmarked &&
>+          visitType != nsINavHistoryService::TRANSITION_BOOKMARK)
>+        bonus += mBookmarkVisitBonus;
nit: Always add the bookmark visit bonus if we haven't yet for the bookmark.

Or something like that..

Also, do we need to tweak the value of bookmark visit bonus? Now every time you visit a link to a bookmarked page, it'll get 150 more frecency for 250 total. That's more than double the current link bonus.

Then why "penalize" bookmark transitions? Clicking a bookmark gives only 150 frecency while clicking a link to a page that happens to be bookmarked gives 250.

Perhaps just if (aIsBookmarked) bonus += mBookmarkVisitBonus

Comment 39

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I agree with Edward, i fear users could complain that their searches are too bounded to bookmarks (only returning bookmarks). maybe the bonus should be 50 now, so it would add a bit, but not too much.

Comment 40

[Dietrich Ayala (:dietrich)]

(In reply to comment #37)
> this is not going to fix other calls to updateFrecency in nsNavBookmarks isn't
> it? I mean bug 455315, those fixes should be quite easy now, so maybe you could
> add them to avoid really minor cases in which the frecency could still be
> zeroed(). Do you want us to fix that apart to provide a specific test?

fine for a followup, marked it as such.

(In reply to comment #38)
...
(In reply to comment #39)
...

agreed, just adding the bookmarkVisitBonus would score bookmark transitions too high. a bump of 50 across the board sounds reasonable. it's also consistent with the current bonus difference between linkVisitBonus and bookmarkVisitBonus.

Comment 41

[Shawn Wilsher :sdwilsh]

(In reply to comment #36)
> (From update of attachment 358922 [details] [diff] [review])
> looks good and i like the approach.
> I think you should also provide a test to check that if those pages are not
> bookmarked but visited (embed download) they don't appear, and that if they are
> bookmarked without visits they appear (unless we already have one test covering
> that).
If they are bookmarked with no visits, they don't have any transition types, so they wouldn't be treated as download/embed anyway.  Added the other test though.

Comment 42

[Dietrich Ayala (:dietrich)]

Comment on attachment 358922 [details] [diff] [review]
v3.2

r=me w/ the changes from irc

Comment 43

[Shawn Wilsher :sdwilsh]

Attachment: v3.3

for checkin

Comment 44

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/9eaf7cf915b6

Comment 45

[Shawn Wilsher :sdwilsh]

This cannot land on branch until bug 412132 gets approval (or if it doesn't, I need to cook up a new patch for branch)

Comment 46

[Boris Zbarsky [:bzbarsky]]

OK, I have good news and bad news.  The good news is that steps to reproduce from comment 10 now work.

The bad news is that in my existing profile with my existing bookmark things still fail.  Searching on "phish" or "auckland" finds various other stuff but NOT that bookmark.

Is that expected (in the sense that the patch affects writes to the DB but not reads from it or something)?

Comment 47

[Shawn Wilsher :sdwilsh]

(In reply to comment #46)
> The bad news is that in my existing profile with my existing bookmark things
> still fail.  Searching on "phish" or "auckland" finds various other stuff but
> NOT that bookmark.
> 
> Is that expected (in the sense that the patch affects writes to the DB but not
> reads from it or something)?
Once you visit the bookmark again, it should start showing up.  We don't recalculate the frecency until you visit it, so it won't fix existing issues.

Comment 48

[Boris Zbarsky [:bzbarsky]]

AHA!  Verified fixed.  Just visiting the bookmark and canceling the dialog makes it happy.  Thank you for aiding my efforts to educate the folks who thing security dialogs are a good idea!

Comment 49

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7f7eb754246e

Comment 50

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090130 Shiretoko/3.1b3pre Ubiquity/0.1.5 ID:20090130030519

Comment 51

[Henrik Skupin [:whimboo][⌚️UTC+1]]

No idea if we already have a similar test on Litmus.

Comment 52

[Shawn Wilsher :sdwilsh]

Don't need a litmus test - this is covered by automated tests.