567824 - gloda's folder property indexingPriority set as function text, not number

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Kent James (:rkent)]

At http://mxr.mozilla.org/comm-central/source/mailnews/db/gloda/modules/index_msg.js#1724 we have the following code:

aFolder.setStringProperty("indexingPriority", Number.toString(aPriority));

This sets the string property to a string representation of a function, not to a number.  We need:

aFolder.setStringProperty("indexingPriority", Number(aPriority).toString());

This does not seem to be noticed in normal operation, because the folder version of indexingPriority is a secondary value, only used if Gloda goes away.
But its value will be completely polluted in the user's profile if this is not fixed, so I hope that if there is a respin of 3.1, then the low risk one-line fix for this can be included. Not likely to block though.

patch to follow once I get my 1.9.2 build working with up to date code.

Comment 1

[Kent James (:rkent)]

OK I give up, can't get a new build to work. The patch is trivial for someone with a build working, so I'm not going to patch this after all.

Comment 2

[Kent James (:rkent)]

Attachment: fix Number usage

Oh never mind, here is the simple patch. But I have not been able run it due to my build problems.

Comment 3

[Kent James (:rkent)]

and I'm setting 3.1 blocking to keep this on the radar.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

The end user effect of this bug, as far as I can tell, is that, for users who turn off indexing for a given folder in the propreties dialog, the next time Gloda gets rebuilt (either because it was removed or because the Gloda schema gets bumped at some point in the future), the folder in question will suddenly start getting indexed at a lower-than-default priority, in conflict with the intent expressed by the user.

Blocking+, as we want this for RC1 build 3 so that we don't end up with any more profiles in the wild with this problem than necessary.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 447198 [details] [diff] [review]
fix Number usage

r=dmose.

It seems clearly correct, the gloda xpcshell-tests all pass with it applied, and there's nothing obviously wrong when playing with the preferences dialog that drives all this.

This shouldn't be very hard to write a test for, either a unit test that just checks property on the message db or a more complete functional test that simulates the failure mode described in my previous comment.  

Since we need to get this landed ASAP in order to kick off RC1 build3, I don't think we want to block on test here, so just setting the in-testsuite? test flag.

Kent, thanks so much for the patch.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

I've pushed it to the 3.1 branch as well, but not the trunk, as the trees are in an odd state.

Comment 7

[Jens Müller (:tessarakt)]

(In reply to comment #4)
> Blocking+, as we want this for RC1 build 3 so that we don't end up with any
> more profiles in the wild with this problem than necessary.

Will you add cleanup code at some point in the future? Or when will it occur, and how to fix the profile?

(Sorry, I have had very bad experiences with bugs in the profile from years ago that suddenly surfaced ...)

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Historically, for bugs like this that have occurred only in non-production (i.e. alpha & beta) versions of the software, Mozilla generally hasn't done that because of lack of resources.  That said, it would be a nice thing to do.   If you're interested in a taking a run at it, I suspect it wouldn't be too difficult
to detect.

Comment 9

[Jens Müller (:tessarakt)]

(In reply to comment #4)
> The end user effect of this bug, as far as I can tell, is that, for users who
> turn off indexing for a given folder in the propreties dialog, 

So, the bug only does harm if I have set that?

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

I believe that's correct, yes.  Note that I haven't gone through and verified by hand that that's the actual failure more.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

"failure mode"

Comment 12

[Mark Banner (:standard8)]

Checked in on trunk: http://hg.mozilla.org/comm-central/rev/551db2c55b20