Places counts auto-refreshes as page visits

VERIFIED FIXED in Firefox 3 beta3

Status

()

P2
major
VERIFIED FIXED
11 years ago
3 years ago

People

(Reporter: deb, Assigned: sdwilsh)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 3 beta3
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

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.
Flags: blocking-firefox3?

Updated

11 years ago
Flags: blocking-firefox3? → blocking-firefox3+
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).
Target Milestone: --- → Firefox 3 M10
Target Milestone: Firefox 3 M10 → Firefox 3 Mx

Updated

11 years ago
Priority: -- → P5

Comment 2

11 years ago
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.
Pretty sure this is fixed now, dietrich?
Priority: P5 → P3
Target Milestone: Firefox 3 Mx → Firefox 3 M11
> 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.
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."
Depends on: 330138
Whiteboard: [patch in bug 402880]
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?
(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.
Created attachment 290407 [details] [diff] [review]
wip (broken out of bug 402880)

- Need to update visit date.
- Needs mochitest to test the scenarios listed in Seth's comment.
Assignee: nobody → dietrich
Status: NEW → ASSIGNED
> 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.
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.

Whiteboard: [patch in bug 402880]
(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.
> 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_*)?
Priority: P3 → P2
(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.
(Assignee)

Comment 16

11 years ago
Created attachment 300160 [details] [diff] [review]
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.
Assignee: dietrich → sdwilsh
Attachment #290407 - Attachment is obsolete: true
Attachment #300160 - Flags: review?(dietrich)
Comment on attachment 300160 [details] [diff] [review]
v1.0

looks good, r=me
Attachment #300160 - Flags: review?(dietrich) → review+
(Assignee)

Comment 18

11 years ago
Created attachment 300254 [details] [diff] [review]
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
Attachment #300160 - Attachment is obsolete: true
Attachment #300254 - Flags: review?(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
Attachment #300254 - Flags: review?(dietrich)
(Assignee)

Comment 20

11 years ago
Created attachment 300292 [details] [diff] [review]
v1.2

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

General cleanup of the test files too.
Attachment #300254 - Attachment is obsolete: true
Target Milestone: Firefox 3 beta3 → Firefox 3 beta4
(Assignee)

Updated

11 years ago
Blocks: 415004
(Assignee)

Comment 21

11 years ago
Created attachment 300560 [details] [diff] [review]
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.
Attachment #300292 - Attachment is obsolete: true
Attachment #300560 - Flags: review?(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.
Attachment #300560 - Flags: review?(dietrich) → review+
(Assignee)

Updated

11 years ago
Whiteboard: [needs new patch][has review]
(Assignee)

Comment 23

11 years ago
Created attachment 300728 [details] [diff] [review]
v1.4

OK, this is it!
Attachment #290349 - Attachment is obsolete: true
Attachment #290350 - Attachment is obsolete: true
Attachment #300560 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Whiteboard: [needs new patch][has review] → [has patch][has review][can land]
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.
Attachment #300728 - Flags: approval1.9b3?
Comment on attachment 300728 [details] [diff] [review]
v1.4

a=beltzner, this is mostly tests
Attachment #300728 - Flags: approval1.9b3?
Attachment #300728 - Flags: approval1.9b3+
Attachment #300728 - Flags: approval1.9+
(Assignee)

Comment 26

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [has patch][has review][can land]
Target Milestone: Firefox 3 beta4 → Firefox 3 beta3
(Assignee)

Updated

11 years ago
Blocks: 330138
No longer depends on: 330138
(Assignee)

Comment 27

11 years ago
See bug 413985 comment 14 about some of these tests being disabled...apparently I saved them wrong.
(Assignee)

Updated

11 years ago
Blocks: 416066
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
Status: RESOLVED → VERIFIED
(Assignee)

Updated

10 years ago
Duplicate of this bug: 238358

Updated

9 years ago
Blocks: 509961
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
(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.
Flags: needinfo?(enndeakin)
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 :)
Flags: needinfo?(enndeakin)
You need to log in before you can comment on or make changes to this bug.