Closed Bug 491719 Opened 11 years ago Closed 11 years ago

Ctrl+H and Ctrl+Maj+H both display an empty history

Categories

(Firefox :: Bookmarks & History, defect, P1)

3.5 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: LpSolit, Assigned: ddahl)

References

Details

(Keywords: regression, verified1.9.1)

Attachments

(2 files, 7 obsolete files)

This bug affects the french locale only:

Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.1b4) Gecko/20090423 Firefox/3.5b4


Neither Ctrl+H nor Ctrl+Maj+H display the history. The list is empty.

I tried with the en-US and German locales, and they are working fine. So this affects fr only.
Flags: blocking-firefox3.5?
This is the same regression as in Bug 420354 "History not shown in sidebar when sorted by date or by date and site".
That is not something that I can fix.
(In reply to comment #1)
> That is not something that I can fix.

The problem doesn't occur in en-US nor in german locales, so isn't it something which is broken in fr locale only, where you have full power on it?
LpSolit, no. This is something to be fixed in the code https://bugzilla.mozilla.org/show_bug.cgi?id=420354#c11 .
By the way, I reproduced the behaviour described in bug 420354. When removing the apostrophe in "Aujourd'hui", the history works fine.
Assignee: bugzilla.fr → nobody
Component: fr / French → Places
Product: Mozilla Localizations → Firefox
QA Contact: benoit.leseul → places
Target Milestone: mozilla1.9.1 → ---
Dietrich: looks like something similar to bug 420354 - can you reassign to someone on your team to handle?
Assignee: nobody → dietrich
Flags: blocking-firefox3.5? → blocking-firefox3.5+
Severity: blocker → major
Assignee: dietrich → ddahl
Priority: -- → P1
Version: unspecified → 3.5 Branch
I can confirm that changing 

"finduri-AgeInDays-is-0=Aujourd'hui"

to 

finduri-AgeInDays-is-0=Aujourd hui

in chrome://places/locale/places.properties

fixes this problem. Still researching the underlying issue. This seems quite similar to bug 42035
Depends on: 390614
Looks like our localizers can inject SQL. See http://mxr.mozilla.org/mozilla1.9.1/source/toolkit/components/places/src/nsNavHistory.cpp#3484

sdwilsh says all of these parameters should be bound properly - let's not trust that the localizers will provide clean strings.
It's not even an issue of trust - we should be flexible enough here to let them use symbols like that.
I am going to assume that binding the parameters will not work as the
parameters are being used as 'column names', not values to be searched by.

can we use an escape mechanism for these strings?
Wait - what problem is the current code trying to solve.  Escaping is error prone, sadly, so I'd like to see us try and engineer a better solution.
i think you can bind param in the column names, is what we were doing, so you should go back using BindAdditionalParameter, looks like i wrongly removed some param in the split history patch, so you can compare my patch and patch in bug 420354, to cook up a patch that will put names in additionalParams and binding in the query.
Not sure why all results are null when getting containers. I am way out of my league here:)
Attachment #377214 - Flags: review?(mak77)
Comment on attachment 377214 [details] [diff] [review]
WIP: fails all history_sidebar tests

we have many :dayTitle... exactly every container has a :dayTitle, so you have to numerate them like :dayTitle1, :dayTitle2... and so on.

the query you are modifying is UNIONED many times
Attachment #377214 - Flags: review?(mak77)
the %s in the query should become dayTitle%d, the number should be an index
and probably you can increase size of mAddParams (that actually is initialized with only 1 entry in addParams.Init(1);)
So - does this mean we cannot actually use mAddParams.Put to bind it correctly?
no, that means you can't .Put("dayTitle"), instead you should put dayTitle1, dayTitle2, dayTitle3... and so on.
error:

toolkit/components/places/src/nsNavHistory.cpp:3609: error: no matching function for call to ‘nsDependentCString::nsDependentCString(nsCAutoString&, unsigned int)’
../../../../dist/include/string/nsTDependentString.h:130: note: candidates are: nsDependentCString::nsDependentCString(const nsCSubstringTuple&)
../../../../dist/include/string/nsTDependentString.h:102: note:                 nsDependentCString::nsDependentCString()
../../../../dist/include/string/nsTDependentString.h:95: note:                 nsDependentCString::nsDependentCString(const nsACString_internal&)
../../../../dist/include/string/nsTDependentString.h:88: note:                 nsDependentCString::nsDependentCString(const char*)
../../../../dist/include/string/nsTDependentString.h:81: note:                 nsDependentCString::nsDependentCString(const char*, PRUint32)
../../../../dist/include/string/nsTDependentString.h:75: note:                 nsDependentCString::nsDependentCString(const char*, const char*)
../../../../dist/include/string/nsTDependentString.h:53: note:                 nsDependentCString::nsDependentCString(const nsDependentCString&)
Attachment #377214 - Attachment is obsolete: true
Attachment #377488 - Flags: review?(mak77)
Attachment #377488 - Flags: review?(mak77)
Comment on attachment 377488 [details] [diff] [review]
WIP 2. More Fail. Changed approach but cannot figure out nsPrintfCString

>+++ b/toolkit/components/places/src/nsNavHistory.cpp
>+      dayTitleName = nsPrintfCString(128, "dayTitle%d", lengthOfAddParamHashTable);
>+      mAddParams.Put(NS_LITERAL_CSTRING(dayTitleName), dateName);
I think you really just want
nsPrintfCString dateTitleName(128, "dayTitle%d", lengthOfAddParamHashTable);
mAddParams.Put(dateTittleName, dateName);
Attached patch WIP - it comiples and then fails (obsolete) — Splinter Review
test_history_sidebar.js errors:

Adding visit to http://mirror5.mozilla.com/b at Sun Oct 26 2008 00:00:00 GMT-0700 (PST)
Adding visit to http://mirror5.mozilla.com/a at Sun Oct 26 2008 00:00:00 GMT-0700 (PST)
Adding visit to http://mirror5.google.com/b at Sun Oct 26 2008 00:00:00 GMT-0700 (PST)
Adding visit to http://mirror5.google.com/a at Sun Oct 26 2008 00:00:00 GMT-0700 (PST)
Found containers:
null
null
null
null
null
null
Found containers:
null
*** exiting
*** TEST-UNEXPECTED-FAIL | /home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-debug/_tests/xpcshell/test_places/unit/test_history_sidebar.js | null == Today
Attachment #377488 - Attachment is obsolete: true
Attachment #377561 - Flags: review?(sdwilsh)
Attachment #377561 - Flags: review?(sdwilsh)
Comment on attachment 377561 [details] [diff] [review]
WIP - it comiples and then fails

a patch is on tryservers waiting to cycle. Clearing request.
tryserver unittests did pass
David please take the patch we were talking about yesterday (you can also find it on tryserver eventually), and only fix the hash size so that it is dynamically correct (you can use the same calculation we have in selectAsDay for NumContainers, hashtable size should probably be numContainers+1, maybe you could need a define or an helper to avoid duplicating that code).
Attached patch v 0.1 All toolkit tests pass (obsolete) — Splinter Review
I duplicated the code that calculates number of containers. Not sure the best approach to making a helper method in this case.
Attachment #377561 - Attachment is obsolete: true
Attachment #377737 - Flags: review?(mak77)
This is a candidate for the trunk patch
Attachment #377737 - Attachment is obsolete: true
Attachment #377784 - Flags: review?(mak77)
Attachment #377737 - Flags: review?(mak77)
Comment on attachment 377784 [details] [diff] [review]
v0.2 all tests pass tweaked a comment

>@@ -4080,17 +4083,27 @@ nsNavHistory::GetQueryResults(nsNavHisto
>   NS_ENSURE_ARG_POINTER(aOptions);
>   NS_ASSERTION(aResults->Count() == 0, "Initial result array must be empty");
>   if (! aQueries.Count())
>     return NS_ERROR_INVALID_ARG;
> 
>   nsCString queryString;
>   PRBool paramsPresent = PR_FALSE;
>   nsNavHistory::StringHash addParams;
>-  addParams.Init(1);
>+
>+  nsNavHistory* history = nsNavHistory::GetHistoryService();
>+  NS_ENSURE_STATE(history);

You are inside a nsNavHistory object, so this is completely useless here

>+  // These are the day containers and catch-all final container.
>+  PRInt32 additionalContainers = 3;

put the 3 in a define please, call it with a meaningful name like ADDITIONAL_DATE_CONTAINERS or something like that, and use the define here and in selectAsDay. 

>+  // We use a guess of the number of months considering all of them 30 days
>+  // long, but we split only the last 6 months.
>+  PRInt32 monthContainers = PR_MIN(6, (history->mExpireDaysMax/30));
>+  PRInt32 numContainers = monthContainers + additionalContainers;

get this through an helper method like ::GetNumDateContainers or ::GetDateContainersCount, both here and in selectAsDay

thanks for taking care of this.
Attachment #377784 - Flags: review?(mak77)
when i say "use the define here" i'm wrong, if you build the helper you will use the define in the helper, we don't need to know number of additional containers there for any other scope.
Mak says r+, asking for one more glance
Attachment #377784 - Attachment is obsolete: true
Attachment #377810 - Flags: review?(mak77)
commented correctly
Attachment #377810 - Attachment is obsolete: true
Attachment #377812 - Flags: review?(mak77)
Attachment #377810 - Flags: review?(mak77)
Comment on attachment 377812 [details] [diff] [review]
0.3.1 Fixed comments as per mak77

>
># HG changeset patch
># User Marco Bonardo <mbonardo@mozilla.com>
># Date 1242346445 -7200
># Node ID af093db9faccff9e5ef2cf3c9917d39d24d44484
># Parent 38d50cc03a7209f8315746fb2aa9ac32363872f0
>[mq]: dateparams.diff
>
>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp
>--- a/toolkit/components/places/src/nsNavHistory.cpp
>+++ b/toolkit/components/places/src/nsNavHistory.cpp
>@@ -211,16 +211,24 @@
> 
> // character-set annotation
> #define CHARSET_ANNO NS_LITERAL_CSTRING("URIProperties/characterSet")
> 
> // We use the TRUNCATE journal mode to reduce the number of fsyncs.  Without
> // this setting we had a Ts hit on Linux.  See bug 460315 for details.
> #define DEFAULT_JOURNAL_MODE "TRUNCATE"
> 
>+// These macros are used when splitting history by date

ending .

>-  for (PRInt32 i = 0; i <= numContainers; i++) {
>+   for (PRInt32 i = 0; i <= DATE_CONT_NUM(history->mExpireDaysMax); i++) {

wrong indentation (you added space before the for)

r=me with tests PASS.
Attachment #377812 - Flags: review?(mak77) → review+
Attached patch for check-inSplinter Review
i'll take care of pushing this.
Attachment #377812 - Attachment is obsolete: true
Thank you Marco!
Whiteboard: [can land]
Passes all tests on 191 branch locally on linux.
Attachment #378113 - Flags: review?(sdwilsh)
BTW: I also tested both patches after changing the locale string in dist/bin/chrome/en-US.jar

I made the test string = Today'foo
http://hg.mozilla.org/mozilla-central/rev/94233e01b40e
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [can land]
Target Milestone: --- → Firefox 3.6a1
Attachment #378113 - Flags: review?(sdwilsh)
Unless there's some major change in the code there's no need for further reviews when porting patches to another branch.
Frederic, could you check on trunk and 1.9.1 if the issue is fixed for the french locale?
Verified fixed for 3.5pre and 3.6a1pre:

 Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.1pre) Gecko/20090521 Shiretoko/3.5pre
 Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.2a1pre) Gecko/20090521 Minefield/3.6a1pre

Thanks!
Status: RESOLVED → VERIFIED
Blocks: 390614
No longer depends on: 390614
Duplicate of this bug: 499110
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.