399606 - Places counts auto-refreshes as page visits

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Deb Richardson [:dria] (plz NEEDINFO)]

I was checking out the new (and awesome) Places menu and noticed that I had two apparently-spurious entries in the "Most Visited Pages" list.  These were pages I had really only visited once, but that auto-refreshed a few hundred times while in the browser (live updates for a Jobs keynote).

Auto-refreshes should not be counted as page visits for this purpose.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

An associated question is: should manual refreshes count? We'll track that in a seperate bug (I'll circle back and update once I've filed).

Comment 2

[Marco Bonardo [:mak]]

i think that manual refresh should not update visit_count, but should only update last visit visit_date. This is because a webdesigner/webmaster/programmer that is working on a website or webapp, will most likely refresh a page hundreds of time a day. And this refreshes will clutter the visit stats.
i want to know what are the address that i visit most, not what are my 10 projects which i'm working on. 
my idea is that every sort of refresh (meta, javascript, user) should only upgrade visit_date, not visit_count.

Comment 3

[Mike Connor [:mconnor]]

Pretty sure this is fixed now, dietrich?

Comment 4

[(not reading, please use [e-mail] instead)]

> Pretty sure this is fixed now, dietrich?

no, not fixed, at least for meta refresh.  (I'll attach a test case.)

note, using location.reload() [like tinderbox] does not have this problem.  (I'll attach a test case for that, too.)

we'd want both test cases in litmus, and any other way of refreshing.

Comment 5

[(not reading, please use [e-mail] instead)]

this might be a dup of bug #330138, where brettw writes:  "This is not something that is easy to add. Meta refreshes are treated very differently from normal refreshes."

Comment 6

[(not reading, please use [e-mail] instead)]

Attachment: location.reload test case

Comment 7

[(not reading, please use [e-mail] instead)]

Attachment: meta refresh test case

Comment 8

[(not reading, please use [e-mail] instead)]

I didn't see the status whiteboard where dietrich wrote:

"[patch in bug 402880]"

dietrich, is this the part of that bug that will address meta refresh?

-  // this call will create the visit and create/update the page entry
+  // If we did find a previous visit, and the URI is the same,
+  // then don't add new visit.
+  if (aReferrer) {
+    PRBool referrerIsSame;
+    aURI->Equals(aReferrer, &referrerIsSame);
+    if (referrerIsSame)
+      return NS_OK;
+  }

Other notes / thoughts:

1) window.location.href = url
2) window.location (same as window.location.href, I think)
3) location.replace(url);
4) location.reload() [already ok, I think]
5) javascript:history.go(0);
6) javascript:history.go(url);
7) http Refresh header

Additionally, depending on how aURI->Equals() works, there might be a way around this code if your url differs by just changing some dummy value on the query string, right?

Comment 9

[Dietrich Ayala (:dietrich)]

(In reply to comment #8)
> I didn't see the status whiteboard where dietrich wrote:
> 
> "[patch in bug 402880]"
> 
> dietrich, is this the part of that bug that will address meta refresh?
> 
> -  // this call will create the visit and create/update the page entry
> +  // If we did find a previous visit, and the URI is the same,
> +  // then don't add new visit.
> +  if (aReferrer) {
> +    PRBool referrerIsSame;
> +    aURI->Equals(aReferrer, &referrerIsSame);
> +    if (referrerIsSame)
> +      return NS_OK;
> +  }
> 
> Other notes / thoughts:
> 
> 1) window.location.href = url
> 2) window.location (same as window.location.href, I think)
> 3) location.replace(url);
> 4) location.reload() [already ok, I think]
> 5) javascript:history.go(0);
> 6) javascript:history.go(url);
> 7) http Refresh header

Ideally, these all use the same Places code-path to increment visits, however I have not confirmed this. I'll write a mochitest that tests each of these scenarios.

> 
> Additionally, depending on how aURI->Equals() works, there might be a way
> around this code if your url differs by just changing some dummy value on the
> query string, right?
> 

Parameter order in the query string should not be significant. If it is, I'd consider that a bug. But to clarify: what do you mean by "dummy value"? If the value of a parameter in the query string changes then it is a different URI.

Comment 10

[Dietrich Ayala (:dietrich)]

Attachment: wip (broken out of bug 402880)

- Need to update visit date.
- Needs mochitest to test the scenarios listed in Seth's comment.

Comment 11

[(not reading, please use [e-mail] instead)]

> Ideally, these all use the same Places code-path to increment visits, however I
> have not confirmed this. I'll write a mochitest that tests each of these
> scenarios.

I was thinking if there were ways around the referrer check

a) window.parent.reload() from another window
b) using frames to reload other frames or the parent

> Parameter order in the query string should not be significant. If it is, I'd
> consider that a bug. But to clarify: what do you mean by "dummy value"? If the
> value of a parameter in the query string changes then it is a different URI.

I'm thinking about sites that might, on a timeout, purposefully do window.location = "http://site.com/index.php?x=y", where "y" changes.

it doesn't have to be a dummy value (one added to the query string specifically to get around our referrer check, in order to affect the visit count), it could be used for legitimate reasons.

Comment 12

[(not reading, please use [e-mail] instead)]

note, some of these issues can / should be turned into spin off issues.

if your patch plugs a common case, like espn's "<meta http-equiv="refresh" content="900" />", then that's good stuff.

Comment 13

[Dietrich Ayala (:dietrich)]

(In reply to comment #11)
> 
> I was thinking if there were ways around the referrer check
> 
> a) window.parent.reload() from another window
> b) using frames to reload other frames or the parent

Maybe if a referrer-less page load is not a top-level page load we should not treat it as a new visit.

> 
> I'm thinking about sites that might, on a timeout, purposefully do
> window.location = "http://site.com/index.php?x=y", where "y" changes.
> 
> it doesn't have to be a dummy value (one added to the query string specifically
> to get around our referrer check, in order to affect the visit count), it could
> be used for legitimate reasons.
> 

Accurately determining intent in this scenario will be difficult.

Comment 14

[(not reading, please use [e-mail] instead)]

> Accurately determining intent in this scenario will be difficult.

agreed, as well as work-arounds/tricks involving a -> b -> a.  (maybe I'm overly worried about a phisher "bombing" autocomplete or sites accidentally impacting autocomplete, due to refresh / reloads.)

dietrich, do we know the transition type when we call addVisitChain(), or can we pass it through (from addVisit())

Can we use that information to distinguish between what the user typed, clicked a link, clicked a bookmark, etc (TRANSITION_*)?

Comment 15

[Dietrich Ayala (:dietrich)]

(In reply to comment #14)
> > Accurately determining intent in this scenario will be difficult.
> 
> agreed, as well as work-arounds/tricks involving a -> b -> a.  (maybe I'm
> overly worried about a phisher "bombing" autocomplete or sites accidentally
> impacting autocomplete, due to refresh / reloads.)
> 

i think this is over-defensive. discounting embeds will remove a large part of the "accidental bombing". intentional bombing by tweaking values in the query string is defeated by frecency: all the visits will have a frequency of 1, so i'm not sure there's a danger from that particular scenario.

> dietrich, do we know the transition type when we call addVisitChain(), or can
> we pass it through (from addVisit())
> 
> Can we use that information to distinguish between what the user typed, clicked
> a link, clicked a bookmark, etc (TRANSITION_*)?
> 

AddVisitChain() is where the transition type is determined.

Comment 16

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Patch in bug applies, and passes the test case in it.  I'll keep writing more tests.

Note, I moved where we did the referrer is the same as the uri test up to have a quicker return (less work) if it was the same.

Comment 17

[Dietrich Ayala (:dietrich)]

Comment on attachment 300160 [details] [diff] [review]
v1.0

looks good, r=me

Comment 18

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

(In reply to comment #8)
> 1) window.location.href = url
have a test
> 2) window.location (same as window.location.href, I think)
have a test
> 3) location.replace(url);
have a test
> 4) location.reload() [already ok, I think]
have a test
> 5) javascript:history.go(0);
need litmus
> 6) javascript:history.go(url);
need litmus
> 7) http Refresh header
fails - filing follow-up

Comment 19

[Dietrich Ayala (:dietrich)]

Comment on attachment 300254 [details] [diff] [review]
v1.1


>Index: toolkit/components/places/tests/browser/399606-window.location.href.html
>===================================================================
>RCS file: toolkit/components/places/tests/browser/399606-window.location.href.html
>diff -N toolkit/components/places/tests/browser/399606-window.location.href.html
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ toolkit/components/places/tests/browser/399606-window.location.href.html	30 Jan 2008 04:50:10 -0000
>@@ -0,0 +1,11 @@
>+<html>
>+<head>
>+<title>foo</title>
>+<script>
>+setTimeout('window.location.href = window.locaction.href',3000);
>+</script>
>+</head>
>+<body>
>+bar
>+</body>
>+</html>

s/locaction/location/

>Index: toolkit/components/places/tests/browser/399606-window.location.html
>===================================================================
>RCS file: toolkit/components/places/tests/browser/399606-window.location.html
>diff -N toolkit/components/places/tests/browser/399606-window.location.html
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ toolkit/components/places/tests/browser/399606-window.location.html	30 Jan 2008 04:50:10 -0000
>@@ -0,0 +1,11 @@
>+<html>
>+<head>
>+<title>foo</title>
>+<script>
>+setTimeout('window.location = window.locaction',3000);
>+</script>
>+</head>
>+<body>
>+bar
>+</body>
>+</html>

ditto

not sure how these are working as expected given this

Comment 20

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

Added a check to make sure we are loading more than once.

General cleanup of the test files too.

Comment 21

[Shawn Wilsher :sdwilsh]

Attachment: v1.3

(In reply to comment #8)
> 1) window.location.href = url
tested and passes
> 2) window.location (same as window.location.href, I think)
tested and passes
> 3) location.replace(url);
tested and passes
> 4) location.reload() [already ok, I think]
tested and passes
> 5) javascript:history.go(0);
tested, but doesn't pass.  Passes when I run the file manually (see bug 415004)
> 6) javascript:history.go(url);
doesn't seem to be valid (didn't work)
> 7) http Refresh header
tested and passes

Also includes the test dietrich had before.

Comment 22

[Dietrich Ayala (:dietrich)]

Comment on attachment 300560 [details] [diff] [review]
v1.3



>+  // If LAZY_LOAD is ever disabled, this function will not be correct

nit: s/LAZY_LOAD/LAZY_ADD/

>+  var loadCount = 0;
>+  function confirm_results() {
>+    var options = histsvc.getNewQueryOptions();
>+    options.resultType = options.RESULTS_AS_VISIT;
>+    options.includeHidden = true;
>+    var query = histsvc.getNewQuery();
>+    var uri = Cc["@mozilla.org/network/io-service;1"].
>+              getService(Ci.nsIIOService).
>+              newURI(TEST_URI, null, null);
>+    dump("query uri is " + uri.spec + "\n");
>+    query.uri = uri;
>+    var result = histsvc.executeQuery(query, options);
>+    var root = result.root;
>+    root.containerOpen = true;
>+    var cc = root.childCount;
>+    // TODO bug 415004
>+    todo_is(cc, 1, "Vist count is what we expect");

nit: s/Vist/Visit/

r=me. thanks for spending the time on this.

Comment 23

[Shawn Wilsher :sdwilsh]

Attachment: v1.4

OK, this is it!

Comment 24

[Reed Loden [:reed]]

Comment on attachment 300728 [details] [diff] [review]
v1.4

I get hit by this bug all the time, mostly in connection with bug 263160. I currently only have three days of history due to this bug, which is massive dataloss for me. If possible, I'd love to get this in for beta 3. Please note the actual change is tiny and that most of this patch is tests to confirm the patch is working.

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 300728 [details] [diff] [review]
v1.4

a=beltzner, this is mostly tests

Comment 26

[Shawn Wilsher :sdwilsh]

Man this was a pain to check in - sooo many files!

Checking in src/nsNavHistory.cpp;
new revision: 1.239; previous revision: 1.238
Checking in tests/Makefile.in;
new revision: 1.3; previous revision: 1.2
Checking in tests/browser/399606-history.go-0.html;
initial revision: 1.1
Checking in tests/browser/399606-httprefresh.html;
initial revision: 1.1
Checking in tests/browser/399606-location.reload.html;
initial revision: 1.1
Checking in tests/browser/399606-location.replace.html;
initial revision: 1.1
Checking in tests/browser/399606-window.location.href.html;
initial revision: 1.1
Checking in tests/browser/399606-window.location.html;
initial revision: 1.1
Checking in tests/browser/Makefile.in;
initial revision: 1.1
Checking in tests/browser/browser_bug413985-history.go-0.js;
initial revision: 1.1
Checking in tests/browser/browser_bug413985-httprefresh.js;
initial revision: 1.1
Checking in tests/browser/browser_bug413985-location.reload.js;
initial revision: 1.1
Checking in tests/browser/browser_bug413985-location.replace.js;
initial revision: 1.1
Checking in tests/browser/browser_bug413985-window.location.href.js;
initial revision: 1.1
Checking in tests/browser/browser_bug413985-window.location.js;
initial revision: 1.1
Checking in tests/unit/test_399606.js;
initial revision: 1.1

Comment 27

[Shawn Wilsher :sdwilsh]

See bug 413985 comment 14 about some of these tests being disabled...apparently I saved them wrong.

Comment 28

[Tracy Walker [:tracy]]

verified with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5) Gecko/2008032606 Firefox/3.0b5
and
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-GB; rv:1.9b5) Gecko/2008032604 Firefox/3.0b5

Comment 30

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 31

[Neil Deakin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=54e559c97ed1

Comment 32

[Tony Mechelynck [:tonymec]]

(In reply to Neil Deakin from comment #31)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=54e559c97ed1

Are you sure you reported this try-run on the right bug? This one was FIXED in 2008.

Comment 33

[Marco Bonardo [:mak]]

it actually ended up here cause autopush extracted the bug number from "imported patch browser_bug399606 ". it was clearly meant for bug 1236554 that is touching the test that was added here.
The Try is green, so not a big deal :)