Closed Bug 420331 Opened 12 years ago Closed 11 years ago

wyciwyg: uris show up in location bar

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3.1b1

People

(Reporter: Mardak, Assigned: dietrich)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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?
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
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?
Attached patch v1 (obsolete) — Splinter Review
Set frecency for wyciwyg: uris to 0.
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #306546 - Flags: review?(seth)
let me check my places.sqlite, but wonder why these urls are not transition embeds?

do you have different weights than the defaults?
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.
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?
(In reply to comment #6)

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

yes.
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 on attachment 306546 [details] [diff] [review]
v1

passing review over to dietrich, but I recommend figuring out how they got in there.
Attachment #306546 - Flags: review?(seth) → review?(dietrich)
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.
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?
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.
Yeah.  Whatever code is involved should be using createExposableURI or whatever it's called.
Depends on: 357776
Keywords: regression
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.
Attachment #306546 - Flags: review?(dietrich)
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?
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
Assignee: edilee → nobody
Component: Location Bar and Autocomplete → Places
QA Contact: location.bar → places
Target Milestone: --- → Firefox 3.1b1
Assignee: nobody → dietrich
Attachment #306546 - Attachment is obsolete: true
Attachment #338151 - Flags: review?(mak77)
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 <darin@meer.net>
>+ *  Dietrich Ayala <dietrich@mozilla.com>
>+ *  Dan Mills <thunder@mozilla.com>
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! ;)
Attachment #338151 - Flags: review?(mak77) → review+
Attached patch for checkinSplinter Review
Attachment #338151 - Attachment is obsolete: true
addVisit() immediately calls canAddURI(), so the transition type doesn't really matter. i did add a test for the addURI() codepath used by docshell.
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?
(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?
The wyciwyg pages should expire as with other history right?
(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
So we're good to check-in the patch as is and just let existing wyciwyg entries expire?
yes, good to go. will check-in someday when the tree is open.
pushed last night: http://hg.mozilla.org/mozilla-central/rev/3db262c66623
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 417704
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
You need to log in before you can comment on or make changes to this bug.