Closed
Bug 420331
Opened 16 years ago
Closed 15 years ago
wyciwyg: uris show up in location bar
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 3.1b1
People
(Reporter: Mardak, Assigned: dietrich)
References
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
3.30 KB,
patch
|
Details | Diff | Splinter Review |
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•16 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•16 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•16 years ago
|
||
Set frecency for wyciwyg: uris to 0.
Comment 4•16 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•16 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•16 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•16 years ago
|
||
(In reply to comment #6) > > Not quite sure how it got there though.. Anybody else using gmail see these? > yes.
Comment 8•16 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•16 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•16 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•16 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?
Comment 12•16 years ago
|
||
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•16 years ago
|
||
Yeah. Whatever code is involved should be using createExposableURI or whatever it's called.
Reporter | ||
Updated•16 years ago
|
Depends on: 357776
Keywords: regression
Assignee | ||
Comment 14•15 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•15 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•15 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•15 years ago
|
Assignee: nobody → dietrich
Assignee | ||
Comment 17•15 years ago
|
||
Attachment #306546 -
Attachment is obsolete: true
Attachment #338151 -
Flags: review?(mak77)
Reporter | ||
Comment 18•15 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•15 years ago
|
||
Attachment #338151 -
Attachment is obsolete: true
Assignee | ||
Comment 20•15 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•15 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?
Comment 22•15 years ago
|
||
(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•15 years ago
|
||
The wyciwyg pages should expire as with other history right?
Comment 24•15 years ago
|
||
(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•15 years ago
|
||
So we're good to check-in the patch as is and just let existing wyciwyg entries expire?
Assignee | ||
Comment 26•15 years ago
|
||
yes, good to go. will check-in someday when the tree is open.
Assignee | ||
Comment 27•15 years ago
|
||
pushed last night: http://hg.mozilla.org/mozilla-central/rev/3db262c66623
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 28•14 years ago
|
||
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.
Description
•