420331 - wyciwyg: uris show up in location bar

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Ed Lee :Mardak]

Should we be showing them? Pretty much all of them are from gmail.

wyciwyg://57/https://mail.google.com/mail/?ui=2&view=js&name=js&ids=...

Do they need a separate transition like downloads? Or just make sure they're frecency 0?

Comment 1

[Seth Spitzer]

I don't think you need a transition, we should probably treat them like we do place: urls, which we exclude.

we also exclude unvisited livemark items, but that's a more complicated process.

I think the trick is to make sure we compute a frecency of 0 for any wyciwyg url

Comment 2

[Seth Spitzer]

see nsNavHistory::FixInvalidFrecenciesForExcludedPlaces() and nsNavHistory::CalculateFrecency()

are we having similar issues with view-source: urls?

maybe the problem is somewhere else, and something changed and we're now adding wyciwyg urls to history when we weren't before?

Comment 3

[Ed Lee :Mardak]

Attachment: v1

Set frecency for wyciwyg: uris to 0.

Comment 4

[Seth Spitzer]

let me check my places.sqlite, but wonder why these urls are not transition embeds?

do you have different weights than the defaults?

Comment 5

[Seth Spitzer]

edward, I don't have any of these urls in my places.sqlite.

what are the steps to reproduce (are those urls for printing?)

it would be worth figuring out how they got in there.

Comment 6

[Ed Lee :Mardak]

All 100+ places have..

visit_count: 1
typed: 0
from_visit: 0
visit_type: 1 (TRANSITION_LINK)

Not quite sure how it got there though.. Anybody else using gmail see these?

Comment 7

[Dietrich Ayala (:dietrich)]

(In reply to comment #6)

> 
> Not quite sure how it got there though.. Anybody else using gmail see these?
> 

yes.

Comment 8

[Seth Spitzer]

I'm rusty, but keep in mind what can happen with session restore.

could these urls be top level urls?  (seems unlikely)

but if they were, when we start up, we reload all tabs and I think we use
TRANSITION_LINK (or another non-ignored TRANSITION, I can't remember which
one).

(I think there is a bug on this, but I'm having trouble finding it.)

This happens a lot with bugzilla and https://bugzilla.mozilla.org/post_bug.cgi,
as you can imagine.

Comment 9

[Seth Spitzer]

Comment on attachment 306546 [details] [diff] [review]
v1

passing review over to dietrich, but I recommend figuring out how they got in there.

Comment 10

[Ed Lee :Mardak]

I deleted all the wyciwyg: uris from my moz_places earlier today and I've already gotten 7 new ones. I haven't done much other than checking mail and letting it sit in the background.

Comment 11

[Ed Lee :Mardak]

So these are perhaps for session history back/forward stuff.. ? Should they not be in moz_places to begin with? Should those get filtered out when adding a page?

Comment 12

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

Neil pointed out on IRC that wyciwyg URIs come from document.open()/write() documents (though they're not exposed to content as such). I don't think they should be in moz_places to begin with - it looks like there's code in docshell that extracts the "actual URI" from them and uses that instead, when adding to session history.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yeah.  Whatever code is involved should be using createExposableURI or whatever it's called.

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 306546 [details] [diff] [review]
v1

i'd rather not add this special case if we can instead address the root cause, and stop these from being added in the first place. removing review request for now.

Comment 15

[Seth Spitzer]

I'm using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.1) Gecko/2008070206 Firefox/3.0.1 and I've got a couple of these wyciwyg urls in my history (both are from gmail)

let me know if there is anything from moz_places that would be helpful.

dietrich, do you think this is worth nominating for Fx 3.1?

Comment 16

[Dietrich Ayala (:dietrich)]

I don't think we'd block on this. For Places end of this, I think we should exclude these up-front here:

http://mxr.mozilla.org/mozilla-central/search?string=all%20bad%20things

Comment 17

[Dietrich Ayala (:dietrich)]

Attachment: deny wyciwyg uris from being added to history

Comment 18

[Ed Lee :Mardak]

Comment on attachment 338151 [details] [diff] [review]
deny wyciwyg uris from being added to history

>diff --git a/toolkit/components/places/tests/unit/test_420331_wyciwyg.js b/toolkit/components/places/tests/unit/test_420331_wyciwyg.js
>new file mode 100644
>+ * The Initial Developer of the Original Code is Google Inc.
>+ * Portions created by the Initial Developer are Copyright (C) 2005
>+ *
>+ * Contributor(s):
>+ *  Darin Fisher <[e-mail]>
>+ *  Dietrich Ayala <[e-mail]>
>+ *  Dan Mills <[e-mail]>
nit: dev, year, contributors

>+function run_test() {
>+  var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
>+                getService(Ci.nsINavHistoryService);
>+  var testURI = uri("wyciwyg://nodontjudgeabookbyitscover");
>+  var placeID = histsvc.addVisit(testURI,
>+                                 Date.now() * 1000,
>+                                 null,
>+                                 histsvc.TRANSITION_TYPED, // user typed in URL bar
>+                                 false, // not redirect
>+                                 0);
>+  do_check_false(placeID > 0);
Arguably the transition isn't really typed as these are showing up automatically. Wouldn't hurt to switch to a plain visited or have both! ;)

Comment 19

[Dietrich Ayala (:dietrich)]

Attachment: for checkin

Comment 20

[Dietrich Ayala (:dietrich)]

addVisit() immediately calls canAddURI(), so the transition type doesn't really matter. i did add a test for the addURI() codepath used by docshell.

Comment 21

[Seth Spitzer]

dietrich,  thanks for the quick fix!  

two things to confirm:

1)  your fix will keep these url out of the location bar, the history sidebar, history in the Library dialog, right?

2)  if a user has these wyciwyg urls in their existing history, this fix won't affect those urls (and they would continue to see them.)  Should we spin off a bug about writing some one-time-only migration code to remove them?

off topic:

1)  should we be excluding moz-icon urls as well?

Comment 22

[Marco Bonardo [:mak]]

(In reply to comment #21)
> dietrich,  thanks for the quick fix!  
> 
> two things to confirm:
> 
> 1)  your fix will keep these url out of the location bar, the history sidebar,
> history in the Library dialog, right?

They will not be added to db at all

> 2)  if a user has these wyciwyg urls in their existing history, this fix won't
> affect those urls (and they would continue to see them.)  Should we spin off a
> bug about writing some one-time-only migration code to remove them?

imho it should be done here since the title of the bug report

> off topic:
> 
> 1)  should we be excluding moz-icon urls as well?

mh, wouldn't be easier moving from a black-list to a white-list?

Comment 23

[Ed Lee :Mardak]

The wyciwyg pages should expire as with other history right?

Comment 24

[Marco Bonardo [:mak]]

(In reply to comment #23)
> The wyciwyg pages should expire as with other history right?

i dont' see why it should not, so probably we could simply wait for it to disappear

Comment 25

[Ed Lee :Mardak]

So we're good to check-in the patch as is and just let existing wyciwyg entries expire?

Comment 26

[Dietrich Ayala (:dietrich)]

yes, good to go. will check-in someday when the tree is open.

Comment 27

[Dietrich Ayala (:dietrich)]

pushed last night: http://hg.mozilla.org/mozilla-central/rev/3db262c66623

Comment 28

[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