749853 - about:* pages should not be stored in history

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Philipp von Weitershausen [:philikon]]

Yes, I see about:home and about: quite a bit, but that doesn't mean they're Top Sites that I'm interested in going to when I tap on the location bar.

Comment 1

[Philipp von Weitershausen [:philikon]]

Yeah, so I finally connected my Android Nightly to my Sync account and oh-my-god are there many about:home entries in Top Sites now.

Comment 2

[:Margaret Leibovic]

(In reply to Philipp von Weitershausen [:philikon] from comment #0)
> Yes, I see about:home and about: quite a bit, but that doesn't mean they're
> Top Sites that I'm interested in going to when I tap on the location bar.

Ian, how do you feel about this?

(In reply to Philipp von Weitershausen [:philikon] from comment #1)
> Yeah, so I finally connected my Android Nightly to my Sync account and
> oh-my-god are there many about:home entries in Top Sites now.

Hrm, this has come up in the past. Bug 741590 should have fixed at least part of this, bug 742381 was a wontfix, but I don't think that should affect you. Can you get a db dump to see where they're all actually coming from? Here's a handy script that will output your DB tables to HTML: http://people.mozilla.com/~rnewman/dumpdbs.py

Comment 3

[Richard Newman [:rnewman]]

> (In reply to Philipp von Weitershausen [:philikon] from comment #1)
> > Yeah, so I finally connected my Android Nightly to my Sync account and
> > oh-my-god are there many about:home entries in Top Sites now.
> 
> Hrm, this has come up in the past...

See also Bug 747699, which will cause reconciling from Fennec to a desktop with existing mobile bookmarks to not work at all.

A workaround (Bug 748898) landed on m-c on 2012-04-27, so should be in the 2012-04-28 Nightly. Won't help you, Philipp; you've already had duping.

Comment 4

[Ian Barlow (:ibarlow)]

This came up a long time ago in bug 710392, and I believe we agreed that the about:home page should never appear in the Top Sites section of about:home, but that we do want to include about: pages in history.

We should never be showing duplicate entries, though.

Comment 5

[Richard Newman [:rnewman]]

(In reply to Ian Barlow (:ibarlow) from comment #4)
> This came up a long time ago in bug 710392, and I believe we agreed that the
> about:home page should never appear in the Top Sites section of about:home,
> but that we do want to include about: pages in history.
> 
> We should never be showing duplicate entries, though.

The top sites query doesn't exclude duplicates, so they'll show up if duplicate data ends up in the DB.

It does (according to the source) filter out about:%.

I think Philipp is talking about the AwesomeBar, rather than Top Sites (on about:home)...?

Comment 6

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #5)
> (In reply to Ian Barlow (:ibarlow) from comment #4)
> > This came up a long time ago in bug 710392, and I believe we agreed that the
> > about:home page should never appear in the Top Sites section of about:home,
> > but that we do want to include about: pages in history.
> > 
> > We should never be showing duplicate entries, though.
> 
> The top sites query doesn't exclude duplicates, so they'll show up if
> duplicate data ends up in the DB.

Duplicate history entries should not exist, but duplicate bookmarks can (bug 741630). I wanted to get a peek a Phil's DB to see exactly what the duplicates are. If they are history entries, that's a worse problem.

Comment 7

[Philipp von Weitershausen [:philikon]]

(In reply to Richard Newman [:rnewman] from comment #5)
> I think Philipp is talking about the AwesomeBar, rather than Top Sites (on
> about:home)...?

Yup.

Comment 8

[Philipp von Weitershausen [:philikon]]

So this was my Nightly profile that I've used ever since I've been on Android. So it was XUL Fennec first, then native, and who knows what migrations and buggy code there was that created stuff. That's fair, I can live with those bugs and understand they happen. So let's talk about what the code *should* do IMHO and then I'll let you guys compare & contrast that with the status quo:

IMHO about:* pages are part of primary UI. We're not interested in their frecency or in syncing them. So don't even record them in history! Less data accrued, less filtering, simpler code.

Comment 9

[:Margaret Leibovic]

Following discussion with mfinkle and ibarlow on IRC, we decided to stop storing about: pages in history. However, you can still bookmark an about: page (some of are default bookmarks are about:home and about:firefox), so those will still show up in top sites as bookmarks, although their rank will be very low since they won't have any history entries.

We could apply the same about: filter we're using on the about:home top sites in the awesomebar if we really want to stop showing them, but this would also change the first run awesomescreen experience. I think we should first try just getting rid of the history entries, then we can explore more extreme measures if these about: bookmarks are still causing problems.

Comment 10

[:Margaret Leibovic]

Attachment: patch

This patch ports over the logic from nsNavHistory::CanAddURI (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#1272), so this will also stop us from saving other unwanted URIs.

I was debating exactly where to do this and decided GlobalHistory looked like the cleanest place, since all calls to update history go through there. However, this means that anyone working directly with BrowserProvider (like sync) wouldn't go through this same check.

Also, I don't see any tests for GlobalHistory, since we do our history testing directly with BrowserProvider. I suppose I could add a robocop test that browses to a bad URI and then makes sure it doesn't get added to the awesomescreen.

And this isn't going to fix existing bad DBs. Should I just not worry about those?

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 620149 [details] [diff] [review]
patch

Review of attachment 620149 [details] [diff] [review]:
-----------------------------------------------------------------

If these are values that should never be allowed in the database -- that is, the rest of Fennec and Sync will assume that there will never be a chrome: link in history -- then this constraint should indeed live in the content provider.

It might *also* apply here, so that you don't incur the expense of a ContentResolver lookup on every ignored history addition, but if it's a constraint it needs to apply everywhere.

But it makes sense to me to lift this into some utility class along with other validation routines -- Validation.isAcceptableHistoryURI(String uri), for example.

::: mobile/android/base/GlobalHistory.java
@@ +115,5 @@
>          }
>          GeckoAppShell.notifyUriVisited(uri);
>      }
>  
> +    // Logic ported from nsNavHistory::CanAddURI

MXR or hg link, perhaps?

@@ +117,5 @@
>      }
>  
> +    // Logic ported from nsNavHistory::CanAddURI
> +    private boolean canAddURI(String uri) {
> +        String scheme = Uri.parse(uri).getScheme();

This seems needlessly expensive for a prefix check on every URL. How about:

// Sanity.
if (string == null || string.length() == 0) {
  return false;
}

// Common case.
if (string.startsWith("http:") ||
    string.startsWith("https:")) {
  return true;
}

// Now either use startsWith for each prefix, or
// extract it using Uri.parse and check for membership in
// a static final Set<String>.
...

Comment 12

[Richard Newman [:rnewman]]

> And this isn't going to fix existing bad DBs. Should I just not worry about
> those?

Again, if this is viewed as a constraint, it should migrate existing databases to match. That would be as simple as a single query on history that flagged matching records as IS_DELETED = 1, modified = now.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

I don't think we need to move this into a DB constraint. I don't want to put that kind of logic in the DB to be honest.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 620149 [details] [diff] [review]
patch


>+    // Logic ported from nsNavHistory::CanAddURI
>+    private boolean canAddURI(String uri) {
>+        String scheme = Uri.parse(uri).getScheme();

This can throw of bad uri strings, so let's add a null check on the uri string first.

Also, it is technically possible to getScheme to return null for relative URLs. I don't think that should happen here, but a null check on the scheme would be simple to add.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

 
> > +    // Logic ported from nsNavHistory::CanAddURI
> > +    private boolean canAddURI(String uri) {
> > +        String scheme = Uri.parse(uri).getScheme();
> 
> This seems needlessly expensive for a prefix check on every URL. How about:
> 
> // Sanity.
> if (string == null || string.length() == 0) {
>   return false;
> }
> 
> // Common case.
> if (string.startsWith("http:") ||
>     string.startsWith("https:")) {
>   return true;
> }

This seems like a good idea for performance. Are we sure that our URLs are always lowercase?

Comment 16

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #15)
>  
> > > +    // Logic ported from nsNavHistory::CanAddURI
> > > +    private boolean canAddURI(String uri) {
> > > +        String scheme = Uri.parse(uri).getScheme();
> > 
> > This seems needlessly expensive for a prefix check on every URL. How about:
> > 
> > // Sanity.
> > if (string == null || string.length() == 0) {
> >   return false;
> > }
> > 
> > // Common case.
> > if (string.startsWith("http:") ||
> >     string.startsWith("https:")) {
> >   return true;
> > }
> 
> This seems like a good idea for performance. Are we sure that our URLs are
> always lowercase?

To be safe, we could always call string.toLowerCase() first.

Comment 17

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #14)
> Comment on attachment 620149 [details] [diff] [review]
> patch
> 
> 
> >+    // Logic ported from nsNavHistory::CanAddURI
> >+    private boolean canAddURI(String uri) {
> >+        String scheme = Uri.parse(uri).getScheme();
> 
> This can throw of bad uri strings, so let's add a null check on the uri
> string first.

It only throws a NPE if the uri is null [1], and my updated patch will already check for that in the sanity-check clause.

[1] http://developer.android.com/reference/android/net/Uri.html#parse(java.lang.String)

Comment 18

[:Margaret Leibovic]

https://hg.mozilla.org/mozilla-central/rev/7fc6248f179d

Comment 19

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #13)
> I don't think we need to move this into a DB constraint. I don't want to put
> that kind of logic in the DB to be honest.

I didn't mean a strict DB constraint; I meant a guaranteed behavior for consumers of the database, just like "we won't insert records with invalid or empty URIs".

Comment 20

[Richard Newman [:rnewman]]

Will steal this for Bug 718153, too. Can delete my duplicate if this ends up shuffling into the CP. Thanks Margaret!

Comment 21

[:Margaret Leibovic]

Comment on attachment 620149 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): n/a
User impact if declined: about:* urls (and other unwanted urls) end up in history, so they show up in top sites/history in the awesome screen
Testing completed (on m-c, etc.): just landed on m/c
Risk to taking this patch (and alternatives if risky): just adds a check before deciding to store a history entry, low risk
String changes made by this patch: n/a

This would be good to get in sooner rather than later, because without this patch users are ending up with unwanted history entries.

Comment 22

[Richard Newman [:rnewman]]

We discussed at triage landing this on Aurora in a few days, perhaps at the same time as Bug 718153, which would be neat and tidy.

Comment 23

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d05ab6c33f93

Comment 24

[Paul Feher]

Verified/Fixed on:
Nightly Fennec 15.0a1 (2012-05-29)
Aurora Fennec 14.0a2 (2012-05-29)
Device: HTC Desire Z
OS: Android 2.3.3

There are no about:* pages stored in history section. They appear in top sites as bookmarks but their rank is very low.