375777 - javascript: urls (and other urls) that I didn't type or click on are in my history sidebar and history menu

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

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

javascript urls are in my urlbar autocomplete and in my history sidebar!

screen shot coming...

Comment 1

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

Attachment: screen shot

Comment 2

[Phil Ringnalda (:philor)]

Attachment: Mozilla 1.7.7 screenshot

Yeah, and?

Well, I have to admit I had that exact same "uh oh, that's not right" reaction myself a week or two ago, but, this is a screenshot of the 1.7.7 suite, oldest thing I had laying around, doing the exact same thing, and the code makes it seems to be what's been intended all along: javascript: URLs that have output are supposed to wind up in history. So, why were we both surprised? Did Fx break it for a while, and only fairly recently (2.0.0.3 does the exact same thing as trunk and my ancient suite) fix it?

Comment 3

[Jesse Ruderman]

I use this all the time.  I don't think there's anything wrong with it, as long as only previously *typed* javascript: URLs are in the autocomplete list.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Could this have been a side effect of switching to places-based global history? IIRC in Firefox 2 these URLs are added to session history but not global history, so they persist for the session but not after a restart (see e.g. bug 359115).

Comment 5

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

fwiw, I'm seeing javascript:"" in my history sidebar, even though I've never typed it.

phils comment about "javascript: URLs that have output are supposed to wind up in history" is interesting.  (for example, javascript:alert("boo"); does not show up in my history.

as for what jesse wrote:  "I don't think there's anything wrong with it, as long
as only previously *typed* javascript: URLs are in the autocomplete list" I think that makes sense.

currently, we exclude embedded visits from the history menu, autocomplete and sidebar, but I'm not sure why javascript:"" is ending up in there, unless that is some sort of dummy url that I've clicked on?

I think we should figure this out before fx 3 ships.

Comment 6

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

in my places.sqlite file, these visits have type 0, which isn't supposed to happen.  (1 - 6 are valid, see http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/public/nsINavHistoryService.idl#1154)

so I've added code to throw me into the debugger if we attempt to call nsNavHistory::InternalAddVisit() with 0 as the transition type.

but going forward, all the places in places SQL where we do: visit_type <> 4 we should do visit_type <> 4 and visit_type <> 0

Comment 7

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

> I've added code to throw me into the debugger

The problem comes from nsNavHistory::AddVisitChain(), because we don't have a referrer.  we don't have a referrer when:

1) opening a new window, and we load the hope page
2) on restart, we reload tabs (and in those pages, we have embedded vists, for example, http://mail.google.com/mail/html/load.html for gmail.)

our current code does:

    if (CheckIsRecentEvent(&mRecentTyped, spec))
      transitionType = nsINavHistoryService::TRANSITION_TYPED;
    else if (CheckIsRecentEvent(&mRecentBookmark, spec))
      transitionType = nsINavHistoryService::TRANSITION_BOOKMARK;

but if it's not a recently typed url or a recently clicked bookmark, transition type is left as 0.

On thing to do

    if (CheckIsRecentEvent(&mRecentTyped, spec))
      transitionType = nsINavHistoryService::TRANSITION_TYPED;
    else if (CheckIsRecentEvent(&mRecentBookmark, spec))
      transitionType = nsINavHistoryService::TRANSITION_BOOKMARK;
    else if (aToplevel)
      transitionType = nsINavHistoryService::TRANSITION_LINK;
    else
      transitionType = nsINavHistoryService::TRANSITION_EMBED;

That would keep "embedded visits" (like to http://mail.google.com/mail/html/load.html for gmail) out of history.  the same would go for javascript:"" (but I need to figure out a test case for that.)  but would add visits for when opening a new window (when you set prefs to load your home page)

Other examples (similar to javascript:"") from ispiked's places.sqlite file:

javascript:false;
javascript:";
javascript:'<html></html>'
javascript:'frame1'
javascript:'frame2'

additionally, I still want to fix our SQL so that we ignore visit_type <> 0 which has creeped into places.sqlite so we don't show those links in the history sidebar, history menu, url bar autocomplete, etc.

Comment 8

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

That sounds like the right approach.

Comment 9

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

Attachment: patch

Comment 10

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

Attachment: history menu and sidebar before

Comment 11

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

Attachment: history menu and sidebar after

Comment 12

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

Attachment: url bar

Comment 13

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

note, for the url bar, this is not fixed.  our currently URL bar query does not join against moz_historyvisits (http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp#272)

The fix for the url bar autocomplete part of this bug is contained in a patch for bug #389491

Comment 14

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

Attachment: from ispiked's places.sqlite, compare sidebar (no javascript urls) to url bar (again, fix coming in bug #389491)

Comment 15

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

steps to reproduce the problem, at least with gmail:

do this in a build without the fix:

1) create new profile (so our prefs are set to load homepage on startup)
2) set http://mail.google.com/mail/ as your home page
3) log into gmail, choose remember me
4) load about:blank
5) clear history
6) exit
7) restart

you should get the load.html in your history sidebar (https://bugzilla.mozilla.org/attachment.cgi?id=274638) because we were not setting that non-top level load as TRANSITION_EMBED.

start up a build with the fix, and you won't see those old loads (as I added visit_type <> 0) and we won't add new loads to your places.sqlite without TRANSITION_EMBED (you'll need SQLite Database Browser to verify that part.)

Comment 16

[Dietrich Ayala (:dietrich)]

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

looks ok, r=me

Comment 17

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

morphing to reflect what this fixes.  I'll log a new bug about the url bar, which will be fixed by the patch in bug #389491

for a places.sqlite to test this bug against, see bug #384677

Comment 19

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

fixed.

Checking in nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHis
tory.cpp
new revision: 1.150; previous revision: 1.149
done

Comment 20

[Al Billings [:abillings - ex-MoCo]]

I used the steps in comment 15 with the build from 6/10 (Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a6pre) Gecko/2007061004 Minefield/3.0a6pre).

I can't reproduce this issue. Is there a specific regression range for this?

All I get in my history menu or sidebar is the inbox page.

Comment 21

[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