wyciwyg: uris show up in location bar

RESOLVED FIXED in Firefox 3.1b1

Status

()

RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: Mardak, Assigned: dietrich)

Tracking

({regression})

Trunk
Firefox 3.1b1
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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

11 years ago
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

11 years ago
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?
(Reporter)

Comment 3

11 years ago
Created attachment 306546 [details] [diff] [review]
v1

Set frecency for wyciwyg: uris to 0.
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #306546 - Flags: review?(seth)

Comment 4

11 years ago
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

11 years ago
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.
(Reporter)

Comment 6

11 years ago
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?
(Assignee)

Comment 7

11 years ago
(In reply to comment #6)

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

yes.

Comment 8

11 years ago
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

11 years ago
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)
(Reporter)

Comment 10

11 years ago
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.
(Reporter)

Comment 11

11 years ago
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.
(Reporter)

Updated

11 years ago
Depends on: 357776
Keywords: regression
(Assignee)

Comment 14

11 years ago
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)

Comment 15

10 years ago
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?
(Assignee)

Comment 16

10 years ago
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)

Updated

10 years ago
Assignee: nobody → dietrich
(Assignee)

Comment 17

10 years ago
Created attachment 338151 [details] [diff] [review]
deny wyciwyg uris from being added to history
Attachment #306546 - Attachment is obsolete: true
Attachment #338151 - Flags: review?(mak77)
(Reporter)

Comment 18

10 years ago
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+
(Assignee)

Comment 19

10 years ago
Created attachment 338167 [details] [diff] [review]
for checkin
Attachment #338151 - Attachment is obsolete: true
(Assignee)

Comment 20

10 years ago
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

10 years ago
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?
(Reporter)

Comment 23

10 years ago
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
(Reporter)

Comment 25

10 years ago
So we're good to check-in the patch as is and just let existing wyciwyg entries expire?
(Assignee)

Comment 26

10 years ago
yes, good to go. will check-in someday when the tree is open.
(Assignee)

Comment 27

10 years ago
pushed last night: http://hg.mozilla.org/mozilla-central/rev/3db262c66623
Status: ASSIGNED → RESOLVED
Last Resolved: 10 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.