813459 - folder flags value have integer overflow when setting the Favorite folder flag (still mixed use of uint32_t and int32_t even after nsMsgFolderFlags.Favorite is defined, and in JS code, ECMA Script returns negative Number to 0x80000000 & 0x80000000)

Status: ASSIGNED

(MailNews Core :: Database, defect)

Description

[:aceman]

I've got this output when watching IntPropertyChanged on a folder:
[xpconnect wrapped (nsISupports, nsIMsgFolder, nsIRDFResource)]FolderFlag from 260 to -2147483388
Source File: chrome://messenger/content/folderWidgets.xml

This is when setting the Favorite Folder flag (0x80000000) in addition to already existing flags having value 260. It looks like the value is overflowing int32 somewhere.

It looks like the flags variable is handled as uint32_t in most places and that should fit all the folder flags. But there are some spots that cast the value to int32_t and I suspect that may cause the overflow.

http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#637

http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#1045

http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#1061

http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#3694

http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#4311 !!!

http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#3752 (unneded cast after fix)

I can't say if the flags are also stored wrongly in the folder or only the notification is sent incorrectly.

So I ask if this is a real problem. I haven't seen any real problems from this but it may hurt in some unexpected case.

My proposal is:
1. either convert the mentioned spots (and look in other files too) to uint32_t
2. or convert all the flag variables to nsMsgFolderFlagType from http://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsMsgFolderFlags.idl, if that is possible.
3. leave nsMsgFolderFlagType at long or convert to uint32_t?

There is one problem at http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#1273 that GetInt32Property and SetInt32Property really take int32_t. I am not sure if we need to store negative values there or this can also be changed to uint32_t. (But for perfection there should be a Set/GetNsMsgFolderFlagTypeProperty ... )

Comment 1

[[e-mail]]

It's not overflow as such, it's just being treated as signed in some cases, so the flag ends up as -2147483648 instead. In fact JavaScript's bitwise operators are all signed, so Components.interfaces.nsMsgFolderFlags.Favorite | 0 is negative. I don't think any of our code actually cares about the sign anyway.

Comment 2

[:aceman]

Attachment: experiment

So this at least makes the internal usage more consistent (uint32_t where possible) and only the storing of the value is still int32_t. Also there is a problem with NotifyIntPropertyChanged() taking int32_t. I expanded it to int64_t (as we may need negative values for some cases). But this is still not enough and I see negative values in JS. Maybe the real storing/retrieving of the value still needs to be converted from GetInt32Property to GetUInt32Property or more future proof GetUInt64Property. I am not sure how hard that is and if we can just change it and it will work with existing profiles correctly.

Comment 3

[[e-mail]]

Comment on attachment 683696 [details] [diff] [review]
experiment

[idl changes need uuid bumps of course]

>   void NotifyIntPropertyChanged(in nsIAtom property,
>-                                in long oldValue,
>-                                in long newValue);
>+                                in int64_t oldValue,
>+                                in int64_t newValue);
I would want to audit all the users before changing to long long. Maybe unsigned long would be better?

>-          folderInfo->GetFlags((int32_t *)&mFlags);
>+          folderInfo->GetFlags(&mFlags);
This is definitely a good move.

>-  int32_t flags = 0;
>+  uint32_t flags = 0;
nsMsgFolderFlagType?

>-  GetInt32PropertyWithToken(m_flagsColumnToken, m_flags);
>+  GetInt32PropertyWithToken(m_flagsColumnToken, (int32_t &) m_flags);
Isn't there a GetUint32PropertyWithToken?

Comment 4

[:aceman]

(In reply to [e-mail] from comment #3)
> Comment on attachment 683696 [details] [diff] [review]
> experiment
> 
> [idl changes need uuid bumps of course]
> 
> >   void NotifyIntPropertyChanged(in nsIAtom property,
> >-                                in long oldValue,
> >-                                in long newValue);
> >+                                in int64_t oldValue,
> >+                                in int64_t newValue);
> I would want to audit all the users before changing to long long. Maybe
> unsigned long would be better?
No, I specifically said that it does not seem correct to just change this to unsigned int as this function is used for all other properties (not just flags) and some of them may need negative values (e.g. I think a folder size of -1 has a special meaning).

> >-  int32_t flags = 0;
> >+  uint32_t flags = 0;
> nsMsgFolderFlagType?
Yes, this would be best, but for now I just converted to uint32_t at the few spots where it was int32_t. I just changed to nsMsgFolderFlagType in some header files. Changing all temp variables and function arguments to nsMsgFolderFlagType would be a much bigger patch.

> >-  GetInt32PropertyWithToken(m_flagsColumnToken, m_flags);
> >+  GetInt32PropertyWithToken(m_flagsColumnToken, (int32_t &) m_flags);
> Isn't there a GetUint32PropertyWithToken?
I think I tried that and there wasn't. But maybe it is not that hard to create it.

I just ask if it is worth it to create a UInt32 one, when maybe soon we will need more flags and get to Int64.

Comment 5

[[e-mail]]

(In reply to aceman from comment #4)
> (In reply to comment #3)
> > (From update of attachment 683696 [details] [diff] [review])
> > >   void NotifyIntPropertyChanged(in nsIAtom property,
> > >-                                in long oldValue,
> > >-                                in long newValue);
> > >+                                in int64_t oldValue,
> > >+                                in int64_t newValue);
> > I would want to audit all the users before changing to long long. Maybe
> > unsigned long would be better?
> No, I specifically said that it does not seem correct to just change this to
> unsigned int as this function is used for all other properties (not just
> flags) and some of them may need negative values (e.g. I think a folder size
> of -1 has a special meaning).
Your "may" wasn't strong enough, which is why I wanted something more specific.

> > >-  GetInt32PropertyWithToken(m_flagsColumnToken, m_flags);
> > >+  GetInt32PropertyWithToken(m_flagsColumnToken, (int32_t &) m_flags);
> > Isn't there a GetUint32PropertyWithToken?
> I think I tried that and there wasn't. But maybe it is not that hard to
> create it.
> 
> I just ask if it is worth it to create a UInt32 one, when maybe soon we will
> need more flags and get to Int64.
Ah, now this is where I can blame you for misreading - there's a Uint32 one with a lower case I, not a UInt32 one with an upper case I!

Of course if we get to Int64 then we'll need NotifyLongPropertyChanged etc.

Comment 6

[:aceman]

(In reply to [e-mail] from comment #5)
> Ah, now this is where I can blame you for misreading - there's a Uint32 one
> with a lower case I, not a UInt32 one with an upper case I!
> 
> Of course if we get to Int64 then we'll need NotifyLongPropertyChanged etc.

Got me :) Yes there is one and it seems unused. So I'll try that.

Comment 7

[:aceman]

Attachment: experiment v2

So it took this much to make me happy to see positive numbers in the JS output. (In the patch there is a line in folderWidgets.xml to output all the property changes. If you add the Folder location button to the toolbar you'll see any folder changes in the error console.) So while now aOld and aNew are nicely positive, aNew & Components.interfaces.nsMsgFolderFlags.Favorite still shows negative as you explained. That may look even more irritating to a watcher like me :) Is there any way to solve this in JS?

Comment 8

[[e-mail]]

Comment on attachment 684144 [details] [diff] [review]
experiment v2

>   void OnItemIntPropertyChanged(in nsIMsgFolder aItem,
>                                 in nsIAtom aProperty,
>-                                in long aOldValue,
>-                                in long aNewValue);
>+                                in int64_t aOldValue,
>+                                in int64_t aNewValue);
I still don't think we should call an int64_t an int. (And we should use long long in idl anyway.)

>+  unsigned long getUint32Property(in string propertyName);
>   void setStringProperty(in string propertyName, in ACString propertyValue);
>   void setInt32Property(in string propertyName, in long propertyValue);
>+  void setUint32Property(in string propertyName, in unsigned long propertyValue);
I don't see why you added these.

>   propertyStr.AppendInt(propertyValue, 16);
>   return SetStringProperty(propertyName, propertyStr);
> }
> 
>+NS_IMETHODIMP nsMsgFolderCacheElement::SetUint32Property(const char *propertyName, uint32_t propertyValue)
>+{
>+  NS_ENSURE_ARG_POINTER(propertyName);
>+  NS_ENSURE_TRUE(m_mdbRow, NS_ERROR_FAILURE);
>+  nsAutoCString propertyStr;
>+  propertyStr.AppendInt(propertyValue, 16);
[As it happens, the base 16 AppendInt uses "%x" which is always unsigned, so you can use SetInt32Property]

>-  element->GetInt32Property("flags", (int32_t *) &mFlags);
>+  element->GetUint32Property("flags", &mFlags);
>   element->GetInt32Property("totalMsgs", &mNumTotalMessages);
>   element->GetInt32Property("totalUnreadMsgs", &mNumUnreadMessages);
>   element->GetInt32Property("pendingUnreadMsgs", &mNumPendingUnreadMessages);
>   element->GetInt32Property("pendingMsgs", &mNumPendingTotalMessages);
>   element->GetInt32Property("expungedBytes", (int32_t *) &mExpungedBytes);
>   element->GetInt32Property("folderSize", (int32_t *) &mFolderSize);
[Looks like some of these should be changed too!]

Comment 9

[:aceman]

(In reply to [e-mail] from comment #8)
> Comment on attachment 684144 [details] [diff] [review]
> experiment v2
> 
> >   void OnItemIntPropertyChanged(in nsIMsgFolder aItem,
> >                                 in nsIAtom aProperty,
> >-                                in long aOldValue,
> >-                                in long aNewValue);
> >+                                in int64_t aOldValue,
> >+                                in int64_t aNewValue);
> I still don't think we should call an int64_t an int. (And we should use
> long long in idl anyway.)
I didn't understand this.

> >+  unsigned long getUint32Property(in string propertyName);
> >   void setStringProperty(in string propertyName, in ACString propertyValue);
> >   void setInt32Property(in string propertyName, in long propertyValue);
> >+  void setUint32Property(in string propertyName, in unsigned long propertyValue);
> I don't see why you added these.
Because these didn't exist. Only the uint32 variants in FolderInfo did exist.

> >   propertyStr.AppendInt(propertyValue, 16);
> >   return SetStringProperty(propertyName, propertyStr);
> > }
> > 
> >+NS_IMETHODIMP nsMsgFolderCacheElement::SetUint32Property(const char *propertyName, uint32_t propertyValue)
> >+{
> >+  NS_ENSURE_ARG_POINTER(propertyName);
> >+  NS_ENSURE_TRUE(m_mdbRow, NS_ERROR_FAILURE);
> >+  nsAutoCString propertyStr;
> >+  propertyStr.AppendInt(propertyValue, 16);
> [As it happens, the base 16 AppendInt uses "%x" which is always unsigned, so
> you can use SetInt32Property]
But SetInt32Property() takes int32 so will mangle the 'above maxint32' value to negative. Will the AppendInt mangle it to positive again?

> >-  element->GetInt32Property("flags", (int32_t *) &mFlags);
> >+  element->GetUint32Property("flags", &mFlags);
> >   element->GetInt32Property("totalMsgs", &mNumTotalMessages);
> >   element->GetInt32Property("totalUnreadMsgs", &mNumUnreadMessages);
> >   element->GetInt32Property("pendingUnreadMsgs", &mNumPendingUnreadMessages);
> >   element->GetInt32Property("pendingMsgs", &mNumPendingTotalMessages);
> >   element->GetInt32Property("expungedBytes", (int32_t *) &mExpungedBytes);
> >   element->GetInt32Property("folderSize", (int32_t *) &mFolderSize);
> [Looks like some of these should be changed too!]
Maybe but I don't know which so I better not touch them. Those are mostly used as int32. But flags was mostly defined as uint32 just sometimes converted to int32.

Comment 10

[Mark Banner (:standard8)]

(In reply to :aceman from comment #9)
> (In reply to [e-mail] from comment #8)
> > Comment on attachment 684144 [details] [diff] [review]
> > experiment v2
> > 
> > >   void OnItemIntPropertyChanged(in nsIMsgFolder aItem,
> > >                                 in nsIAtom aProperty,
> > >-                                in long aOldValue,
> > >-                                in long aNewValue);
> > >+                                in int64_t aOldValue,
> > >+                                in int64_t aNewValue);
> > I still don't think we should call an int64_t an int. (And we should use
> > long long in idl anyway.)
> I didn't understand this.

The function name is OnItemIntPropertyChanged, so that would probably be better reflected as OnItemInt64PropertyChanged (based on the other function names). I think I agree with Neil here that we should set expectations correctly.

Comment 11

[:aceman]

Really Int64 even though there would be not Int32 variant?
And there are tons of callers that would need renamed:
http://mxr.mozilla.org/comm-central/search?string=OnItemIntPropertyChanged&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

Many are in JS so those probably do not care much about the size.

Comment 12

[Mark Banner (:standard8)]

Well, you could have both an Int64 and an Int32, but obviously, we'd need to go through which callers require which, which is probably also the audit that I think Neil was referring to earlier.

Comment 13

[:aceman]

Why do we need both 32 and 64bit variants? Is it a perf problem if we just go up to 64bit for all the OnItemIntPropertyChanged functions? Does any listener care for exact size? I don't think the JS ones do. There are no CPP ones. And the implementations of OnItemIntPropertyChanged across the various files can be audited if they handle int64 arguments passed to them.

I imagine that all the JS callers would need to listen to both variants of these notifiers so it would add complexity.

Comment 14

[Mark Banner (:standard8)]

I think convention wise it would just be nice to call these Int64, as we do with many of our other functions.

Comment 15

[:aceman]

Then we should have named them Int32 in the first place, not Int. I think having the Int names is a nice opportunity to silently increase the size without renaming :)

Comment 16

[:aceman]

OK, it seems we will need the *Long* version for folder size in bug 789679 too, so let's revive this bug and make the *Long* infrastructure:)

I just think this will break many extensions that expect flag changes to come via OnItemIntPropertyChanged and they will now come via OnItemLongPropertyChanged . If they even work as they will not have any OnItemLongPropertyChanged handler.

Comment 17

[WADA:World Anti-bad-Duping Agency]

(In reply to [e-mail] from comment #1)
> In fact JavaScript's bitwise operators are all signed, (snip)

It's not true.
JavaScript "bitwise operation" is simply "pure bitwise operation".
When several bytes used for bitwise operation in JavaScript is shown, the several bytes are shown as signed integer.

> so the flag ends up as -2147483648 instead.

Why flags value itself shown by JavaScript is important?
When setting bit or combination of bits,
  Object.flags = Object.flags | (flag1|flag2| ... |flagN) ) ;
and, when checking bit or combination of bits,
  if( 0 == Object.flags & (flag1|flag2| ... |flagN) ) all bits is Off;
  else one of bits is On;
that's all.

Because Favarite is defined as most significant bit(left most bit) if 32bits data, if flags is 32bits data, JavaScript shows it as number represented in 32bits signed integer.
> http://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsMsgFolderFlags.idl#104
> 104   const nsMsgFolderFlagType Favorite        = 0x80000000;
If more bits are needed, flags is defined as 64bits, and new flag bits are defined at left side.
> const nsMsgFolderFlagType    Favorite    =  0x80000000;
> const nsMsgFolderFlagType64  newFlagBit1 = 0x100000000;
> const nsMsgFolderFlagType64  newFlagBit2 = 0x200000000;
  
> I don't think any of our code actually cares about the sign anyway.

Code like following is standard in Thunderbird and is used in many places?
  if( 0 < Object.flags & (flag1|flag2| ... |flagN) ) one of bits is On;
  else all bits is Off;
If so, simplest solution is;
   change flags to 64bits, never use left most bit of the 64bits data

Comment 18

[[e-mail]]

(In reply to WADA from comment #17)
> (In reply to comment #1)
> > In fact JavaScript's bitwise operators are all signed, (snip)
> 
> It's not true.
> JavaScript "bitwise operation" is simply "pure bitwise operation".
> When several bytes used for bitwise operation in JavaScript is shown, the
> several bytes are shown as signed integer.

But it casts its arguments to int32_t first. (So it's useless for 64 bit flags...)

> Because Favarite is defined as most significant bit(left most bit) if 32bits
> data, if flags is 32bits data, JavaScript shows it as number represented in
> 32bits signed integer.
> > http://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsMsgFolderFlags.idl#104
> > 104   const nsMsgFolderFlagType Favorite        = 0x80000000;
nsMsgFolderFlagType is uint32_t, so Components.interfaces.nsMsgFolderFlags.Favorite appears to be 2147483648, but whenever you use a JavaScript bitwise operator on it it turns into -2147483648. As far as I know this is purely cosmetic, so perhaps the best approach is to make nsMsgFolderFlagType into int32_t?

Comment 19

[Joshua Cranmer [:jcranmer]]

(In reply to WADA from comment #17)
> (In reply to [e-mail] from comment #1)
> > In fact JavaScript's bitwise operators are all signed, (snip)
> 
> It's not true.

Au contraire, ECMA-262 would mind to disagree:
The production A : A @ B, where @ is one of the bitwise operators in the productions above, is evaluated as follows:
[...]
5. Let lnum be ToInt32(lval).
6. Let rnum be ToInt32(rval).
7. Return the result of applying the bitwise operator @ to lnum and rnum. The result is a signed 32 bit integer.

> If so, simplest solution is;
>    change flags to 64bits, never use left most bit of the 64bits data

That would produce surprising results in JavaScript:
let a = 0x100000000; /* This can't be represented in 32 bits... */
(a & a) == a; /* Returns false */
[Verify this for yourself with Scratchpad.]

Note that this also holds for a = 0x80000000 as well (!).

Keeping this as a signed 32-bit integer may seem weird, but if we don't, we screw up JS code.

Comment 20

[WADA:World Anti-bad-Duping Agency]

(In reply to Joshua Cranmer [:jcranmer] from comment #19)
> 5. Let lnum be ToInt32(lval).
> 6. Let rnum be ToInt32(rval).
> 7. Return the result of applying the bitwise operator @ to lnum and rnum.
> The result is a signed 32 bit integer.

Number in bitwise operation looks still 32bits in ECMA Script...
No improvement or enhancement to 64bits in JavaScript?
If No, we need to split 64bits to two 32bits data to use JavaScript's logical AND and OR. Or we have to add 32bits msgFolder.flags_2, with reserving left most bit for safety.

> (In reply to WADA from comment #17)
> > (In reply to [e-mail] from comment #1)
> > > In fact JavaScript's bitwise operators are all signed, (snip)
> > It's not true.
> Au contraire, ECMA-262 would mind to disagree:
> The production A : A @ B, where @ is one of the bitwise operators in the
> productions above, is evaluated as follows:
> [...]
> 5. Let lnum be ToInt32(lval).
> 6. Let rnum be ToInt32(rval).
> 7. Return the result of applying the bitwise operator @ to lnum and rnum.
> The result is a signed 32 bit integer.

It's pure "32bits bitwise operation", although limited to 32bits.

If HexaDecimal display of flags value is needed, it's obtained by code like following.
What's bad?

> http://stackoverflow.com/questions/57803/how-to-convert-decimal-to-hex-in-javascript#answer-3689638
  function decimalToHex(d) {
    var hex = Number(d).toString(16);
    hex = "000000".substr(0, 6 - hex.length) + hex; 
    return hex;
  }
  // or "#000000".substr (0, 7 - hex.length) + hex;
  // or "0x000000".substr(0, 8 - hex.length) + hex;

Comment 21

[WADA:World Anti-bad-Duping Agency]

If code like "if(0<flags&FlagBits) one of bits is On;else all bits is Off;" is standard of Tb, I prefer following to "making it to int32_t elsewhere". 
  - Add 32bits flags_2, push out Favarite bit to flags_2,
    reserve left most bit of flags_2 for our safety,
    and reserve 0x80000000 of flags for our safety.
  - Add flags/flags_2 check method.
    msgFolder.CheckFlag(FlagBits_1,Result_1,FlagBits_2,Result_2)
      Result_1 = msgFolder.flags & FlagBits_1; 
      if(FlagBits_2) Result_2 = msgFolder.flags_2 & FlagBits_2;
      else           Result_2 = 0; 
      return ( 0!=Result_1 || 0!=Result_2 );
      // true : one of bits is On
      // false: all bits is Off
      Or, return Result_2*(2**32) + Result_1;
      // 0<Result_N is garanteed, because left most bit is reserved 
  => No need to change any code other than code uses Favarite bit,
     and code uses Favarite bit is pretty rare :-)
  => Because left most bit is reserved, next two is same.
     - if(0==flagsX&Flagbit) all bits is Off;   else one of bits is On;
     - if(0<flagsX&Flagbit)  one of bits is On; else all bits is Off;

Comment 22

[WADA:World Anti-bad-Duping Agency]

For "JavaScript, Favarite bit in msgFolder.flags, negative flags value in ECMA Script" relevant part only in this bug.
If I'm still misunderstanding problem, ignore this comment, please. And sorry for lengthy comment. 

Flag display.
Following is better for flag bit check, because JavaScript adds "sign" when negative value.
  function D2X(d) {var hex=Number(d).toString(16);return hex;}
  function D2Xa(d){var hex=Number(Math.abs(d)).toString(16);return hex;}
  var a =  0x80000000;
  var b = 0x180000000;
  var c = -2147483648; // 0x80000000 if 32bits signed integer
  var x = (a & a); var y = (b & b);
  var s="/";
  var txt  = D2X(c)  +s+ D2X(x)  +s+ D2X(y);  // txt is; -80000000 / -80000000 / -8000000
  var txta = D2Xa(c) +s+ D2Xa(x) +s+ D2Xa(y); // txt is;  80000000 /  80000000 /  8000000

About your "In fact JavaScript's bitwise operators are all signed", and problem in JavaScript due to negative number like next.
> (a & a) == a; /* Returns false */
> [Verify this for yourself with Scratchpad.]
> Note that this also holds for a = 0x80000000 as well (!).

I think phenoenon in JavaScript(ECMA Script) is;
  var a =  0x80000000; Held as "double-precision 64-bit format IEEE 754 values".
                       (So, 64bits bitwise operation will perhaps not be supported)
                       If integer, one of -Infinity, -(largest positive) to -1, 0, 1 to largest positive, Infinity
  var b = 0x180000000;
  var c = -2147483648; // 0x80000000 if 32bits signed integer, 2's complement)
                       // In ECMA Script, equivallent to "32bits unsigned 0x80000000" + "- sign".

  var x = (a & a); value of "a" is copied to 32bit unsigned integer(sign bit is ignored),
                   and all the 32bits is AND'ed by ECMA script.
                   (so, I called pure 32bits operation)
                   And, it looks all 32bits is copied to "x" which is "double-precision 64-bit format IEEE 754 values",
                   (32bits result value is treated as 32bits unsigned integer)
                   and sign bit is also copied(treated as 32bits signed integer).
                   It sounds different from following in ECMA Script spec.
                     7. Return the result of applying the bitwise operator @ to lnum and rnum.
                        The result is a signed 32 bit integer.
                     Perhaps it's your "In fact JavaScript's bitwise operators are all signed".
                   It may be differece of JavaScript from ECMA Script.
                   However, I believe it's following.
                     0x80000000 in 32bits signed integer == -2147483648 in decial,
                     so "2147483648" is saved in "x" with sign bit=1.

  var x_eq_a = (x==a); // "x" is negative number, "a" is positive number, so x_eq_a is false.

  var y = (b & b);
  var z = (c & c);

  var txt_1 = D2X(a) +s+ D2X(b) +s+ D2X(c);
  // =>  80000000 / 180000000 / -8000000
  var txt_2 = D2X(x) +s+ D2X(y) +s+ D2X(z);
  // => -80000000 / -80000000 / -8000000

When msgFolder.flags value == 0x80000000, and when x=msgFolder.flags is executed, JavaScript shows "x" as -2147483648.
I think it's following.
  0x80000000 in 32bits signed integer == -2147483648 in decimal,
  so "2147483648" is saved in "x" with sign bit=1. 
When x = -2147483648 in ECMA Script, and when msgFolder.flags=x is executed, I think following occurs.
  JavaScript convert the "2147483648 with sign bit=1" to "0x80000000 of 32bits sined integer",
  and stores it to msgFolder.flags.

I think;
- As far as all 32bits of number data of a property of "XPCOM Wrapped Object" is copied to number ofject of ECMA Script,
  problem due to negative value in ECMA Script won't occur by bitwaise operation of ECMA script.
- As far as all 32bits in number object of ECMA Script is copied back to a property of "XPCOM Wrapped Object",
  sign bit in double-precision 64-bit floating point data in ECMA Script doesn't corrupt flag bits.
- So, problem in JS Code is following only.
  When left most bit of 32bits data in flags is On,
  check like next produces wrong bit handling, because ECMA Script shows it as nevative value,
    if(0<flags&CheckBits) one of bits is On; else all bits is Off;
  It should be next always.
    if(0==flags&CheckBits) all bits is Off; else one of bits is On;
  If this is problem in JavaScript code in Tb,
  "if( ... Math.abs(flags&CheckBits) ) in common routine" is a simplest solution.
 
Some questions about "int32_t or uint_32_t in C++ code".

Does attribute of msgFolder.flag(signed 32bits or unsigned 32bits) depend on "int32_t use or uint32_t use in each C++ code"?
It depends on Class definition of msgFolder.flags only, or spec of ECMA Script(32bits numeric data is 32bits signed data" like one), doesnn't it?
Is it a cause of problem like "Favarite folder setting is somehow lost"?

Comment 23

[:aceman]

If we converted all code to use GetFlag that just returns bool accordint to the flag state I would be OK with any signed/unsigned values representing the flags. But if we expose the internal representation of the flags (the integer, which I do not like much) I am bothered with showing the value as negative.

I have a patch in progress that consistently uses flags internally as uint32_t but exposes them via OnItemLongPropertyChanged which has the value as int64_t.

Comment 24

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #23)
> But if we expose the internal representation of the flags (the
> integer, which I do not like much) 
> I am bothered with showing the value as negative.

"flags" is simply a word, a 32bits data, which is usually placed at word boundary in memory. And, any C/Assembler code can interprete it as int32_t or uint32_t or a word or 4 8bits-ascii chars.

In ECMA JavaScript, "result of bitwise operation is interpreted as 32bits signed integer" is ECMA Script's spec. And, the result value is held as Number, integer in this case, and it's double precision floating point data in ECMA Script. So, if left most bit of the 32bits data is On, sign bit of double precision floating point data is set to On, then shown as negative number. This is SPEC and design of ECMA Script.

What is actual problem in Tb when 32bits bitwise operation result is shown as negative number due to "left most bit=On in 32bits flag data" in JavaScript?
It's merely ugly or confusing for some peoples, and it's only when displayed as decimal number at somewhere, isn't it?
That is SPEC of ECMA Script. If negative number in ECMA Script is shown to people for flag bits, and if it's bad for people, I believe it's fault of who created code which displays the value as-is :-)

If flags, set of flag bits, like msgFolder.flags, "displaying it as positive HexaDecimal" is sufficient, isn't it?
> function Flag_D2X(d){
> // clear sign bit of Number generated from left most bit of 32bits flag data
> // and show it in HexaDecimal 
> var hex=Number(Math.abs(d)).toString(16);
> // convert to 8 digits representation with preceding ZERO, because 32bits flag data
> "0x000000".substr(0, 8 - hex.length) + hex;
> return hex;}

> I have a patch in progress that consistently uses flags internally as
> uint32_t but exposes them via OnItemLongPropertyChanged
> which has the value as int64_t.

I also think "cosistent use of int32_t in C++ code" is mandatory, because "loss of left most bit" can occur if var_of_uint32_t=var_of_int32_t is executed at some place, and it's usually hard to avoid because "detection of difference of sign bit by code viewing" is usually very hard.

> exposes them via OnItemLongPropertyChanged which has the value as int64_t.

If the 64bits data is accessed via "XPCOM Wrapped Object" by JS code, and if bitewise operation is needed on each flag bit, (i) split to two 32bits data, (ii) bitwise operation for each 32 bits, (iii) merge two results of two bitwise operation, is required due to restriction of bitwise operation in ECMA Script.
Is 64bits LongProperty already used in Tb for flag data like msgFolder?

Comment 25

[:aceman]

(In reply to WADA from comment #24)
> What is actual problem in Tb when 32bits bitwise operation result is shown
> as negative number due to "left most bit=On in 32bits flag data" in
> JavaScript?
> It's merely ugly or confusing for some peoples, and it's only when displayed
> as decimal number at somewhere, isn't it?

Exactly, it just confused me :) However whenever anybody notices this we may need to audit and explain why it is OK to display a negative number.
If it is possible for us to make it positive, nobody will ask needlessly.

> > I have a patch in progress that consistently uses flags internally as
> > uint32_t but exposes them via OnItemLongPropertyChanged
> > which has the value as int64_t.
> 
> I also think "cosistent use of int32_t in C++ code" is mandatory, because
> "loss of left most bit" can occur if var_of_uint32_t=var_of_int32_t is
> executed at some place, and it's usually hard to avoid because "detection of
> difference of sign bit by code viewing" is usually very hard.

Exactly. We probably got away with it because we only casted (int32_t) uint32_t and were not assigning variables like you write. But who knows.

> > exposes them via OnItemLongPropertyChanged which has the value as int64_t.
> 
> If the 64bits data is accessed via "XPCOM Wrapped Object" by JS code, and if
> bitewise operation is needed on each flag bit, (i) split to two 32bits data,
> (ii) bitwise operation for each 32 bits, (iii) merge two results of two
> bitwise operation, is required due to restriction of bitwise operation in
> ECMA Script.
I am told Javascript can handle numbers up to about 53 bits. So we could up the internal variable to 64bits as long as we do not use more than 53 of them. But that is not needed yet, we just have 32 flags, so we just solve 31-bit -> 32-bits here, so I keep the uint32_t flags declaration as is.

> Is 64bits LongProperty already used in Tb for flag data like msgFolder?

It is not existing yet. I am building it, see the discussion in previous comments. But we will need it for other folder properties too, like folder size (remember >4GB).

Comment 26

[[e-mail]]

(In reply to aceman from comment #25)
> I am told Javascript can handle numbers up to about 53 bits.
Only from the point of view of the smallest integer that it cannot represent exactly is 54 bits long. Any flag bit-twiddling is limited to 32 bits.

Comment 27

[:aceman]

(In reply to [e-mail] from comment #26)
Any flag bit-twiddling is limited to 32 bits.

I know about bit shifts. But does this limit also cover & and | operators?

Comment 28

[:aceman]

OK, looks like that is the case:
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Operators/Bitwise_Operators

Another point in the case, that we should hide the internal representation of flags and not let JS code doing bitwise operations on the returned integer. We are limited to 32 flags until we solve this. Or make 2 32-bit attributes as WADA proposed.

Now I am not sure we want to make the OnItemLongProperty be 64bit as we can't ever use it to pass more than 32 bits.

Comment 29

[:aceman]

It's not that bad, can we kill the flags attribute? :)
http://mxr.mozilla.org/comm-central/search?string=.flags+%26&find=%2Fmail&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

Comment 30

[:Irving Reid (No longer working on Firefox)]

Changing folder flags to something else at this point could break a lot of Thunderbird extensions.

If we need future extension, we can look into either re-using obsolete flag bits if we have any, or add another word of flags. We have at least one other place in the TB tree where an interface has more than one word of flags; I can't remember for sure, but I think it's the mail server interface.

I think we should close this bug.

Comment 31

[Joshua Cranmer [:jcranmer]]

(In reply to :Irving Reid from comment #30)
> Changing folder flags to something else at this point could break a lot of
> Thunderbird extensions.
> 
> If we need future extension, we can look into either re-using obsolete flag
> bits if we have any, or add another word of flags. We have at least one
> other place in the TB tree where an interface has more than one word of
> flags; I can't remember for sure, but I think it's the mail server interface.
> 
> I think we should close this bug.

To second this opinion:

The main issue I've had is that people (specifically, add-on authors whom we have effectively no control over) who use the bitshifts for flags are going to have surprising results that are not intuitive, especially for new flags.

Moving to a 64-bit flags value is a non-starter: people use flagA | flagB even if they don't use flags & flag directly.

Comment 32

[WADA:World Anti-bad-Duping Agency]

Attachment: JavaScript sample to get HexaDecimal dump of msgFolder.flags and result of bitwise operation

(In reply to Joshua Cranmer [:jcranmer] from comment #19)
> (In reply to WADA from comment #17)
> > (In reply to [e-mail] from comment #1)
> > > In fact JavaScript's bitwise operators are all signed, (snip)
> > It's not true.
> Au contraire, ECMA-262 would mind to disagree:
> The production A : A @ B, where @ is one of the bitwise operators in
> the productions above, is evaluated as follows:
> [...]
> 5. Let lnum be ToInt32(lval).
> 6. Let rnum be ToInt32(rval).
> 7. Return the result of applying the bitwise operator @ to lnum and rnum.
> The result is a signed 32 bit integer.

It's PURE "32bits SIGNED integer operation" in fact, as you say...
I was confused "32bits signed integer data generated by bitwise operation" with "decimal string shown by JavaScript after conversion from the 32bits signed integer".
I was confused "32bits data in XPCOM Wrapped Object property" with "numeric data of Number object in JavaScript which is converted from the 32bits data".
Sorry for my confusion.

Because 32bits data is interpreted as "32bits signed integer" and is shown as JavaScript Number Object data, 2's complement value for the Number object is needed to obtain hexa decimal representation of original 32bits flag and bitwise operation result.
Why can't we simply dump 32bits data in HexaDecimal?
Who forced assembler like coding in JavaScript to us?

Attached is JavaScript code sample to get HexaDecimal dump of flag data and bitwise operation result.
(1) Two HexaDecimal dump routines, A routine returns 2's complement.
>   D2X   : hex = Number(d).toString(16);
>   D2XXX : if(0<=d){var ddd = d; }else{var ddd = Math.pow(2,32) + d; }
>           hex = Number(ddd).toString(16);
>   COMP  : if(0<=d){var ddd = d; }else{var ddd = Math.pow(2,32) + d; } return ddd;
(2) Four test data(JavaScript Number).
> Number data in decimal
> a = 4294967287 , b = 2155905152 , c = -2147483647 , d = -1
> HexaDecimal dump of JavaScript Number Object by D2X()
> a = FFFFFFF7 , b = 80808080 , c = -7FFFFFFF , d = -1
> HexaDecimal dump of JavaScript Number Object by D2XXX() == 2's complement
> a = FFFFFFF7 , b = 80808080 , c = 80000001 , d = FFFFFFFF
(3) Result of bitwise operation.
    X & a, 2's complement of X & a, Math.abs(X) & a, is checked.
> bitwise operation result : HexaDecimal by D2X()
> a & a = -9 , COMP(a) & a = -9 , abs(a) & a = -9
> b & a = -7F7F7F80 , COMP(b) & a = -7F7F7F80 , abs(b) & a = -7F7F7F80
> c & a = -7FFFFFFF , COMP(c) & a = -7FFFFFFF , abs(c) & a =  7FFFFFF7
> d & a = -9        , COMP(d) & a = -9        , abs(d) & a = 1
> bitwise operation result : HexaDecimal by D2XXX(), If negative, 2**32 + decimal is shown.
> a & a = FFFFFFF7  , COMP(a) & a = FFFFFFF7  , abs(a) & a = FFFFFFF7
> b & a = 80808080  , COMP(b) & a = 80808080  , abs(b) & a = 80808080
> c & a = 80000001  , COMP(c) & a = 80000001  , abs(c) & a = 7FFFFFF7
> d & a = FFFFFFF7  , COMP(d) & a = FFFFFFF7  , abs(d) & a = 1
> bitwise operation result : Decimal of bitwise operation result
> a & a = -9          , COMP(a) & a = -9          , abs(a) & a = -9
> b & a = -2139062144 , COMP(b) & a = -2139062144 , abs(b) & a = -2139062144
> c & a = -2147483647 , COMP(c) & a = -2147483647 , abs(c) & a =  2147483639
> d & a = -9          , COMP(d) & a = -9          , abs(d) & a = 1

(A) When Number Object value is negative, 2's complement is used by bitwise operation of ECMA Script.
(B) When 32bits flag value is shown as neative Number of JavaScript, original bit pattern of 32bits data is 2's complement of the negative Number of JavaScript.
(C) To get HexaDecimal dump of negative msgFolder.flags value and neative bitwise operation result in ECMA Script, Number(2**32+ddd).toString(16) is needed.
(D) Because conversion to 2's complement occurs in bitwise operation of ECMA Script if Number in JavaScript is negative, negative Number what is set by user shouldn't be used for bitwise operation, in order to avoid confusion produced by ourself. When X=-1 and Y=-3, these are "0x0...001 with sign bit=On" and "0x0...011 with sign bit=On" in Number object. If ddd=Math.abs(X)&Math.abs(Y) is used, it's ddd = "0x0...001 with sign bit=Off" & "0x0...011 with sign bit=Off", then HexaDecimal dump by Number(ddd).toString(16), 0x1, is easy to understad and it's usually same as our ordinal expectation when getting dump data.

Function like following and D2X_Flag_32(msgFolder.flags), D2X_Flag_32( msgFolder.flags & (MsgFlag1 | ... | MsgFlagN) ) is perhaps convenient to get HexaDecimal dump of flag data and bitwise operation result.
> function D2X_Flag_32(d)
> {
>   if(0<=d){var ddd = d; }else(0-Math.pow(2,32)<d && d<0){var ddd=Math.pow(2,32)+d; }else{ ddd=null; }
>   var hex = Number(ddd).toString(16);
>   var hexdec = "00000000".substr(0, 8 - hex.length) + hex;
>   return hexdec.toUpperCase(); // or return "0x0" +hexdec.toUpperCase(); 
> }

Comment 33

[WADA:World Anti-bad-Duping Agency]

Following is source line contains nsMsgFolderFlags.Favorite.
> http://mxr.mozilla.org/comm-central/search?string=nsMsgFolderFlags.Favorite&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

I think change of at least following code is needed. JS code for test is better changed too.
http://mxr.mozilla.org/comm-central/source/mail/base/content/folderPane.js#1258
> 1257         for each (let folder in ftv._enumerateFolders) {
> 1258           if (folder.flags & nsMsgFolderFlags.Favorite)
>     ==> if ( 0 != (folder.flags & nsMsgFolderFlags.Favorite) )
> 1259             faves.push(new ftvItem(folder));
> 1260         }
http://mxr.mozilla.org/comm-central/source/mailnews/db/gloda/modules/datastore.js#2046
> 2046     else if (aFolder.flags & Ci.nsMsgFolderFlags.Favorite)
>      ==> else if ( 0 != (aFolder.flags & Ci.nsMsgFolderFlags.Favorite) )
> 2047       indexingPriority = GlodaFolder.prototype.kIndexingFavoritePriority;

Comment 34

[WADA:World Anti-bad-Duping Agency]

Following is valeus defined in Ci.nsMsgFolderFlags(name is obtained by for(nm in Ci.nsMsgFolderFlags)).
  ----------------------------
  Newsgroup       =        1, 
  NewsHost        =        2, 
  Mail            =        4, 
  Directory       =        8, 
  Elided          =       10, 
  Virtual         =       20, 
  Subscribed      =       40, 
  Unused2         =       80, 
  Trash           =      100, 
  SentMail        =      200, 
  Drafts          =      400, 
  Queue           =      800, 
  Inbox           =     1000, 
  ImapBox         =     2000, 
  Archive         =     4000, 
  Unused1         =     8000, 
  Unused4         =    10000, 
  GotNew          =    20000, 
  ImapServer      =    40000, 
  ImapPersonal    =    80000, 
  ImapPublic      =   100000, 
  ImapOtherUser   =   200000, 
  Templates       =   400000, 
  PersonalShared  =   800000, 
  ImapNoselect    =  1000000, 
  CreatedOffline  =  2000000, 
  ImapNoinferiors =  4000000, 
  Offline         =  8000000, 
  OfflineEvents   = 10000000, 
  CheckNew        = 20000000
  Junk            = 40000000, 
  Favorite        = 80000000, 
  ----------------------------
  SpecialUse      = 40405F00, 
  ----------------------------

Fortunately, three unused bits are seen(Unused2,Unused1,Unused4).
I think following is lowest-cost solution.
(1) Move Favorite bit to Unused4 position
(2) Reserve 0x80000000. Name = ReservedToAvoidProblemsDueToSignBit
(3) In JS migration code relevant to Favorite flag,
>   if( (msgFolder.flags & Ci.nsMsgFolderFlags.ReservedToAvoidProblemsDueToSignBit) < 0 ) {
>      Reset ReservedToAvoidProblemsDueToSignBit bit.
>      and Set Ci.nsMsgFolderFlags.Favorite bit; // current Unused4
>   }
We still have two unused bits, so "two flag words" can be postponed.
And, beacuse left most bit is reserved, we can avoid chaos by "Negative number after bitwise operation due to spec of ECMA Script", without change of any current code.
Problem in above solution:
  Favorite bit is lost by fall back to older Tb version.
I think it's never significant and I believe simple note in Release Notes is sufficient.
- If you do fall back, set Favorite folder manually again, please.
- To add-on developers, Unused4 bit is now used as Favarite bit. Don't use, please.

Comment 35

[:aceman]

WADA, that would be a nice solution. But I am not sure if it is acceptable for extensions.

We have even more unused flags than those 3 you found, see bug 858993.

Comment 36

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #35)
> But I am not sure if it is acceptable for extensions.

(a) For add-on developer, including add-on relevant to Favorite folder.
    Because bit bosition itself is irrelevant and name of "Favorite"
    only is important, no problem, as far as Tb's migrator code
    correctly moves Favorite bit position in msgFolder.flags data.
    And, if no access to nsMsgFolderFlags.Favorite bit, no influence.
(a-1) Add-on by JavaScript, flags bit access via Ci.nsMsgFolderFlags.
    They can continue to use any of next;
    - if(msgFolder.flags&Ci.nsMsgFolderFlags.AnyBits)    Bits are On;
    - if(msgFolder.flags&Ci.nsMsgFolderFlags.AnyBits!=0) Bits are On;
(a-2) Add-on by C++, Favorite bit access using nsMsgFolderFlags
      interface, instead of by flag bit definition in header file.
    No problem in C++ code. They can use any of int32_t and uint32_t
    freely for msgFolder.flags at any place.  
(a-3) Add-on by C++, with nsMsgFolderFlags definition in xxx.h file.
    He has to change his xxx.h, and has to recompile modules.
    They also can use any of int32_t and uint32_t freely for
    msgFolder.flags at any place.
(b) For add-on developer who are using Unused4 bit in his add-on.
    He should use Unused1 or Unused2 instead of removed Unused4.

Comment 37

[:aceman]

1. OK, so moving specifically 'flags' attribute to 64bit length was voted down here.

2. Moving it NotifyIntPropertyChanged() to unsigned long (uint32_t) is also a no go as some other attributes using it may need the negative values support (being int32_t now).

So what do you guys this about WADAs solution? I just wonder how to migrate existing folders as they will have the flag set in their msf databases and we read it in as value 80000000. The database reading code would need to special case this value and convert it to the new value. The same couls be temporarily done in the main methods setting the flags.

Comment 38

[:aceman]

So it appears to me that in bug 789679 comment 118 I got approval from standard8 to at least upgrade the nsIFolderListener::OnItemIntPropertyChanged to int64_t for use by e.g. folder size property. Can I prepare a patch for just that and not touching the flags property? Do we have agreement on that?

Comment 39

[Joshua Cranmer [:jcranmer]]

(In reply to :aceman from comment #38)
> So it appears to me that in bug 789679 comment 118 I got approval from
> standard8 to at least upgrade the
> nsIFolderListener::OnItemIntPropertyChanged to int64_t for use by e.g.
> folder size property. Can I prepare a patch for just that and not touching
> the flags property? Do we have agreement on that?

I plead apathy on this issue.

Comment 40

[:Irving Reid (No longer working on Firefox)]

We can change the OnItemIntPropertyChanged API to 64 bits; this would cause API breakage for binary add-ons, but it might be worth taking that pain.

I dislike exposing boolean properties of things (like folder flags) as integers that need to be interpreted with bit-twiddling logic; I believe that any future flags should be defined as attributes on the IDL api rather than trying to cram them into our current 'flags' number.

Because coercion back and forth between int32 and uint32 does not lose data, I don't think we should make any significant API changes *in this case*. Making the code more obviously unsigned where feasible is fine, and future APIs should (in my opinion) avoid both the OnItem*PropertyChanged API family, and the use of bitwise flags.

Comment 41

[Mark Banner (:standard8)]

I'm happy with shifting to 64 bit as that seems to make sense.

Comment 42

[:Irving Reid (No longer working on Firefox)]

(In reply to Mark Banner (:standard8) from comment #41)
> I'm happy with shifting to 64 bit as that seems to make sense.

For folder size, that's great. For flags, because JS only does bitwise operations on the low 32 bits, making the flags word 64-bit creates a loaded footgun for the first developer who adds one more flag thinking that there's lots of room for more bits. The only benefit we get, as far as I can tell, is that when we print the flags word as an integer it doesn't come out negative when the high bit flag is set.

Comment 43

[:aceman]

In comment 38 I promised to not touch flags and keep them internally as 32bit (the flag word) so nothing should change even if they get passed to JS via a 64bit interface. I hope that is possible.

So after changing OnItemIntPropertyChanged to 64bit, I only want it to use for bug 789679 (i.e. folder size). And the bug here will stay open (if it actually is a bug).

Is that satisfactory?

Comment 44

[:Irving Reid (No longer working on Firefox)]

(In reply to :aceman from comment #43)
> In comment 38 I promised to not touch flags and keep them internally as
> 32bit (the flag word) so nothing should change even if they get passed to JS
> via a 64bit interface. I hope that is possible.
> 
> So after changing OnItemIntPropertyChanged to 64bit, I only want it to use
> for bug 789679 (i.e. folder size). And the bug here will stay open (if it
> actually is a bug).
> 
> Is that satisfactory?

Yes.

Comment 45

[:aceman]

(In reply to :Irving Reid from comment #40)
> I dislike exposing boolean properties of things (like folder flags) as
> integers that need to be interpreted with bit-twiddling logic; I believe
> that any future flags should be defined as attributes on the IDL api rather
> than trying to cram them into our current 'flags' number.

I would be OK with NOT exposing the flags property so that "folder.flags & flag" is no longer possible.
And everything would be queried via .getFlag(flag). However, if we currently fetch the .flags and then test multiple flags successively, that involves only one call into the C++ code (and XPCOM boundary). Converting that to multiple .getFlag() calls would be more expensive. So we would have to find a properly performant substitute.

Comment 46

[:aceman]

Attachment: patch v3 (just convert the listener)

Aryx, could you make a try run with this? Thanks.

Comment 47

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed to Thunderbird-Try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=fad9041cd09f

Comment 48

[:aceman]

Attachment: patch v3.1

Yeah, fails on platforms other than linux. This should fix it.

Comment 49

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed to Tb-Try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=5e57ec4dc79e

Comment 50

[:aceman]

Comment on attachment 8500690 [details] [diff] [review]
patch v3.1

This intentionally does not upgrade any folder properties to 64bit, it should just make it possible to do it in the respective bugs.
I understood it that we do not need to keep versions of the internal functions for 32bit and 64bit variant and that we can just change the definitions of the existing *IntPropertyChanged functions.
Can you look at this if it is enough? I didn't notice any new problems on the try server caused by this patch. But it seems to me we do not use them much in core code. Maybe extensions hook into them a bit more?

Comment 51

[Kent James (:rkent)]

Attachment: Part 2, make sizeOnDisk int64

As a followup to attachment 8500690 [details] [diff] [review], this changes folderSize to int64. Perhaps you had a similar patch elsewhere? In any case, I think we should use int64 and not uint64 for this.

Comment 52

[:aceman]

Yes, this is in https://bug789679.bugzilla.mozilla.org/attachment.cgi?id=738666 .

Comment 53

[WADA:World Anti-bad-Duping Agency]

Is this bug for issue of "32bits integer use in folderSize"?
This bug is for following issues, isn't it?
   (a) comment #33 in JS code when Favorite flag is on.
   (b) mixed use of uint32_t and int32_t for Favorite flag handling in C++/C code, as written in bug summary.
Even though "problem in ECMA Script's Number object for 64bits integer in C++ code/C++ object" is found in in this bug, I believe that "change folderSize to 64bits integer" should be proposed in other bug than this bug, in order to avoid future confusions.

Comment 54

[WADA:World Anti-bad-Duping Agency]

Correction of my comment #33.
"if(0x80000000&0x80000000)" in JavaScript was identical to "if(0!=0x80000000&0x80000000)", because result of AND operation is Number Object of JavaScript.
> javascript: var x=y=0x80000000; var z=x&y; if(x&y) alert("true/"+x+"/"+z); else alert("false/"+x+"/"+z); 
> => true/2147484648/-2147484648
So, code like "if (folder.flags & nsMsgFolderFlags.Favorite)" won't produce any problem, unless "folder.flags & nsMsgFolderFlags.Favorite" is printed(funny negative number is printed).

Sorry for my confusion.

Comment 55

[:aceman]

Comment on attachment 8515510 [details] [diff] [review]
Part 2, make sizeOnDisk int64

Review of attachment 8515510 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this is fine (matches my work in bug 789679), but does not belong into this bug.

Comment 56

[:aceman]

Comment on attachment 8515510 [details] [diff] [review]
Part 2, make sizeOnDisk int64

I hide this patch as agreed with rkent, as a similar patch is in bug 789679 and now unbitrotted.

Comment 57

[:aceman]

(In reply to WADA from comment #54)
> So, code like "if (folder.flags & nsMsgFolderFlags.Favorite)" won't produce
> any problem, unless "folder.flags & nsMsgFolderFlags.Favorite" is
> printed(funny negative number is printed).

Yes, the whole problem was reported by me only because the number looks weird when printed (for debugging purposes). So the bug is about discussion if we can make this display any better. It seems internally everything works fine even with these int32_t/uin32_t conversions.

Comment 58

[Kent James (:rkent)]

Comment on attachment 8500690 [details] [diff] [review]
patch v3.1

I'm seeing the following error on a try server build https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=56be541c2e3a

Log

TB Linux x86-64 try-comm-central leak test build on 2014-11-11 13:20:29 PST for push 56be541c2e3a

/builds/slave/tb-try-c-cen-l64-d-00000000000/build/mailnews/base/util/nsMsgUtils.cpp:2453:11: error: redefinition of 'nsMsgKey msgKeyFromInt(uint64_t)'
/builds/slave/tb-try-c-cen-l64-d-00000000000/build/mailnews/base/util/nsMsgUtils.cpp:2448:11: error: 'nsMsgKey msgKeyFromInt(long unsigned int)' previously defined here

Comment 59

[Kent James (:rkent)]

Sorry, that was for the wrong patch. It's on bug 793865.

Comment 60

[[e-mail]]

Comment on attachment 8500690 [details] [diff] [review]
patch v3.1

> NS_IMETHODIMP
> nsMsgFolderDataSource::OnItemIntPropertyChanged(nsIMsgFolder *folder,
>                                                 nsIAtom *property,
>-                                                int32_t oldValue,
>-                                                int32_t newValue)
>+                                                int64_t oldValue,
>+                                                int64_t newValue)
> {
>   nsCOMPtr<nsIRDFResource> resource(do_QueryInterface(folder));
>   if (kTotalMessagesAtom == property)
>     OnTotalMessagePropertyChanged(resource, oldValue, newValue);
>   else if (kTotalUnreadMessagesAtom == property)
>     OnUnreadMessagePropertyChanged(resource, oldValue, newValue);
>   else if (kFolderSizeAtom == property)
>     OnFolderSizePropertyChanged(resource, oldValue, newValue);
What happens to the methods called here?

Comment 61

[Kent James (:rkent)]

(In reply to [e-mail] from comment #60)
> Comment on attachment 8500690 [details] [diff] [review]
> patch v3.1
> 
> > NS_IMETHODIMP
> > nsMsgFolderDataSource::OnItemIntPropertyChanged(nsIMsgFolder *folder,
> >                                                 nsIAtom *property,
> >-                                                int32_t oldValue,
> >-                                                int32_t newValue)
> >+                                                int64_t oldValue,
> >+                                                int64_t newValue)
> > {
> >   nsCOMPtr<nsIRDFResource> resource(do_QueryInterface(folder));
> >   if (kTotalMessagesAtom == property)
> >     OnTotalMessagePropertyChanged(resource, oldValue, newValue);
> >   else if (kTotalUnreadMessagesAtom == property)
> >     OnUnreadMessagePropertyChanged(resource, oldValue, newValue);
> >   else if (kFolderSizeAtom == property)
> >     OnFolderSizePropertyChanged(resource, oldValue, newValue);
> What happens to the methods called here?

OnFolderSizePropertyChanged has been changed to int64_t. The others remain using int32_t. I suppose these calls are implicit downcasts?

Comment 62

[[e-mail]]

(In reply to Kent James from comment #61)
> OnFolderSizePropertyChanged has been changed to int64_t.

Where? Is my tree out of date?

Comment 63

[Kent James (:rkent)]

(In reply to [e-mail] from comment #62)
> (In reply to Kent James from comment #61)
> > OnFolderSizePropertyChanged has been changed to int64_t.
> 
> Where? Is my tree out of date?

OK sorry, that is in Bug 789679 which is a followup to this one.

This current bug is a predecessor of a lot of ongoing work in folderSize.  You can also see try run at https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-central&revision=498602673d2e that includes this bug as well as the current version of the followup bugs.

Comment 64

[[e-mail]]

(In reply to Kent James from comment #63)
> (In reply to comment #62)
> > (In reply to Kent James from comment #61)
> > > OnFolderSizePropertyChanged has been changed to int64_t.
> > 
> > Where? Is my tree out of date?
> 
> OK sorry, that is in Bug 789679 which is a followup to this one.

OK fair enough. But one more question if you don't mind: do the get/setInt64Property changes really belong here or in a separate patch or bug?

Comment 65

[:aceman]

Yeah, the new functions are not used in this patch, but are used in https://bug789679.bugzilla.mozilla.org/attachment.cgi?id=8517705 . So we can move them if you want.

Comment 66

[[e-mail]]

Comment on attachment 8500690 [details] [diff] [review]
patch v3.1

Either that or at least separate them out into their own commit.

Comment 67

[:aceman]

Attachment: [checked in, tm:tb36] convert OnItemIntPropertyChanged listener to 64bit - patch v3.2

Sure, no problem. Thanks.
Carrying over r+ from Neil and Irving.

Comment 68

[Magnus Melin [:mkmelin]]

Comment on attachment 8523811 [details] [diff] [review]
[checked in, tm:tb36] convert OnItemIntPropertyChanged listener to 64bit - patch v3.2

https://hg.mozilla.org/comm-central/rev/f54859b9c9d3

Comment 69

[Kent James (:rkent)]

What is still needed on this bug?

Comment 70

[:aceman]

The original problem as reported was not fixed in anyway. But it is not critical and not needed for 4GB folders. You do not need to consider it. Thanks.

Comment 71

[Kent James (:rkent)]

I interpret your comment as this is not blocking bug 789679, so I removed blocking.

Comment 72

[:aceman]

Yes, the one patch done here was needed for >4GB folders, but the bug as a whole is not.

Comment 73

[WADA:World Anti-bad-Duping Agency]

(In reply to WADA from comment #53)
> Even though "problem in ECMA Script's Number object for 64bits integer in C++ code/C++ object" is found in in this bug, (snip)

I've opened bug 1135559 for it.

Comment 74

[Wayne Mery (:wsmwk)]

And this one?

Comment 75

[Magnus Melin [:mkmelin]]

Seems like something may be still at least theorietically wrong here.

FWIW, the whole nsMsgFolderFlagType typedef is a bit odd - https://searchfox.org/comm-central/search?q=nsMsgFolderFlagType&path= - not used anywhere and since you're supposed to be able to use flags together with each other, I don't think the parameters should be converted to using it either. Potentially yes for the single-flag cases, but rather futile...
https://searchfox.org/comm-central/rev/5cda2cb985aaf24a65cd766f6ba3fa3332df0d25/mailnews/base/public/nsMsgFolderFlags.idl#8-13