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

Status: VERIFIED FIXED

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

Description

[Frédéric Buclin]

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.

Comment 1

[Cédric Corazza]

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.

Comment 2

[Frédéric Buclin]

(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?

Comment 3

[Cédric Corazza]

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.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Dietrich: looks like something similar to bug 420354 - can you reassign to someone on your team to handle?

Comment 5

[David Dahl :ddahl]

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

Comment 6

[David Dahl :ddahl]

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.

Comment 7

[Shawn Wilsher :sdwilsh]

It's not even an issue of trust - we should be flexible enough here to let them use symbols like that.

Comment 8

[David Dahl :ddahl]

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?

Comment 9

[Shawn Wilsher :sdwilsh]

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.

Comment 10

[Marco Bonardo [:mak]]

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.

Comment 11

[David Dahl :ddahl]

Attachment: WIP: fails all history_sidebar tests

Not sure why all results are null when getting containers. I am way out of my league here:)

Comment 12

[Marco Bonardo [:mak]]

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

Comment 13

[Marco Bonardo [:mak]]

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);)

Comment 14

[David Dahl :ddahl]

So - does this mean we cannot actually use mAddParams.Put to bind it correctly?

Comment 15

[Marco Bonardo [:mak]]

no, that means you can't .Put("dayTitle"), instead you should put dayTitle1, dayTitle2, dayTitle3... and so on.

Comment 16

[David Dahl :ddahl]

Attachment: WIP 2. More Fail. Changed approach but cannot figure out nsPrintfCString

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&)

Comment 17

[Shawn Wilsher :sdwilsh]

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);

Comment 18

[David Dahl :ddahl]

Attachment: WIP - it comiples and then fails

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

Comment 19

[Marco Bonardo [:mak]]

Comment on attachment 377561 [details] [diff] [review]
WIP - it comiples and then fails

a patch is on tryservers waiting to cycle. Clearing request.

Comment 20

[Marco Bonardo [:mak]]

tryserver unittests did pass

Comment 21

[Marco Bonardo [:mak]]

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).

Comment 22

[David Dahl :ddahl]

Attachment: v 0.1 All toolkit tests pass

I duplicated the code that calculates number of containers. Not sure the best approach to making a helper method in this case.

Comment 23

[David Dahl :ddahl]

Attachment: v0.2 all tests pass tweaked a comment

This is a candidate for the trunk patch

Comment 24

[Marco Bonardo [:mak]]

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.

Comment 25

[Marco Bonardo [:mak]]

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.

Comment 26

[David Dahl :ddahl]

Attachment: v0.3 All tests pass, added macros via marco

Mak says r+, asking for one more glance

Comment 27

[David Dahl :ddahl]

Attachment: 0.3.1 Fixed comments as per mak77

commented correctly

Comment 28

[Marco Bonardo [:mak]]

Comment on attachment 377812 [details] [diff] [review]
0.3.1 Fixed comments as per mak77

>
># HG changeset patch
># User Marco Bonardo <[e-mail]>
># 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.

Comment 29

[Marco Bonardo [:mak]]

Attachment: for check-in

i'll take care of pushing this.

Comment 30

[David Dahl :ddahl]

Thank you Marco!

Comment 31

[David Dahl :ddahl]

Attachment: 0.1 -191 branch patch

Passes all tests on 191 branch locally on linux.

Comment 32

[David Dahl :ddahl]

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

Comment 33

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/94233e01b40e

Comment 34

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/53650d7a1ce8

Comment 35

[Marco Bonardo [:mak]]

Unless there's some major change in the code there's no need for further reviews when porting patches to another branch.

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Frederic, could you check on trunk and 1.9.1 if the issue is fixed for the french locale?

Comment 37

[Frédéric Buclin]

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!

Comment 39

[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