733535 - selecting an empty mailing list causes an error "cards[i] is null"

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[:aceman]

When selecting an empty mailinglist in the left AB tree (the right pane with cards is now empty) causes this error:

Error: An error occurred updating the cmd_delete command: [Exception... "'[JavaScript Error: "cards[i] is null" {file: "chrome://messenger/content/addressbook/abResultsPane.js" line: 157}]' when calling method: [nsIController::isCommandEnabled]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://global/content/globalOverlay.js :: goUpdateCommand :: line 80"  data: yes]
Source File: chrome://global/content/globalOverlay.js
Line: 86

For some reason, cards.length == 1, but dereferencing cards[i] (i=0..length-1) produces the error. Maybe the cards are just hidden but still exist and the focus is on one of them?

Comment 1

[ISHIKAWA, Chiaki]

Attachment: card-is-null-fix.patch

(In reply to :aceman from comment #0)
> When selecting an empty mailinglist in the left AB tree (the right pane with
> cards is now empty) causes this error:
> 
> Error: An error occurred updating the cmd_delete command: [Exception...
> "'[JavaScript Error: "cards[i] is null" {file:
> "chrome://messenger/content/addressbook/abResultsPane.js" line: 157}]' when
> calling method: [nsIController::isCommandEnabled]"  nsresult: "0x80570021
> (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame ::
> chrome://global/content/globalOverlay.js :: goUpdateCommand :: line 80" 
> data: yes]
> Source File: chrome://global/content/globalOverlay.js
> Line: 86
> 
> For some reason, cards.length == 1, but dereferencing cards[i]
> (i=0..length-1) produces the error. Maybe the cards are just hidden but
> still exist and the focus is on one of them?

Hi, I also noticed this one in "make mozmill" session log.
Will the attached fix work (I suppose it will, but I am not sure if this
is just a bandage that hides the problem and not really
addressing the deeper issue. [I am not sure what happens *IF* all the cards[i] 
are null even if cards.length is positive. Maybe the routine will
return bogus/unintended/improper value? ]).

TIA

Comment 2

[ISHIKAWA, Chiaki]

Thank you for adding the feedback flag, aceman.

I wonder if there has been a change in sematics of array-like data object access.

Please see the Bug 900824.

There, an array (or a sequence?) is referenced and just like here, the length is one, but
when referenced the 0-th element is actually a null pointer and so JS runtime
throws an error. There I produced a bandage fix just like the one I posted here.

Looking at the similarity of the issues, I wonder if there has been a semantic change in the underlying data structure, etc. (Or maybe the semantics of GUI library/toolkit that
returns such array-like object.)


TIA

Comment 3

[:aceman]

I don't think there is a problem in array semantics.
I think in both cases the array contains the 0th member and the member is null ([null]). It is not that the array is null or empty ([]) but reports one member.

It maybe a scenario like this:
var card = GetCardWithPropertiesX();
var array = [];
array.push(card);

And not noticing GetCardWithPropertiesX returned null (e.g. no such card found), so still pushing the return value into the array.
In our case GetCardWithPropertiesX is not GetSelectedAbCards, but this whole flawed logic may be inside GetSelectedAbCards() as that returns the whole array. It should probably return an empty array if the selection is somehow bad.
If you can, please check line http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/content/abResultsPane.js#187, see if abView.getCardFromRow(j) can fail and in that case do not add its result to the cards array. It may be that there are selection areas that do not represent valid cards.

Comment 4

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #3)
> I don't think there is a problem in array semantics.
> I think in both cases the array contains the 0th member and the member is
> null ([null]). It is not that the array is null or empty ([]) but reports
> one member.
> 
   [...]
> If you can, please check line
> http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/content/
> abResultsPane.js#187, see if abView.getCardFromRow(j) can fail and in that
> case do not add its result to the cards array. It may be that there are
> selection areas that do not represent valid cards.

Thank you for the comment. I have modified so that failed getCardFrom(j) value (i.e. null) is not inserted into cards array and am running |make mozmill| test
to see if this solves the issue.

TIA

Comment 5

[ISHIKAWA, Chiaki]

Attachment: card-is-null-fix.patch (Take Two)

Following aceman's suggestion, the second version of the patch is produced.
The value filled into |cards| into array is checked and only non-null value
is inserted. 
At the end, the length of the array, filled with non-null values, may be shorter than originally anticipated,
and so the new array with proper length is created and the non-null values
are copied to it and then it is returned instead of the original flawed |cards| array.

I ran |make mozmill| test locally, and the "cards[i] is null" error is no longer seen (!)

TIA

TIA

Comment 6

[ISHIKAWA, Chiaki]

Attachment: card-is-null-fix.patch (take Three)

Sorry, take two is not complete. 
One line change for if(tmp) [from if(!tmp)] got
buried in ANOTHER patch for a problem on which I was working
in parallel.

So here is a corrected complete version.

TIA

Comment 7

[:aceman]

Comment on attachment 786283 [details] [diff] [review]
card-is-null-fix.patch (take Three)

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

::: mailnews/addrbook/content/abResultsPane.js
@@ +121,5 @@
>    var mailingListCnt = 0;
>    var cardCnt = 0;
>    for (var i = 0; i < count; i++) {
> +    // there was a case where cards[i] was null.
> +    // it is now fixed in GetSeletedAbCards().

I am not sure this comment is needed. But mconley will decide.

@@ +187,5 @@
>      abView.selection.getRangeAt(i,start,end);
> +    for (j = start.value; j <= end.value; ++j) {
> +      // avoid null assignment. GetRangeAt() may be buggy.
> +      var tmp = abView.getCardFromRow(j);
> +      if(tmp)	

Space after 'if' please. Also it seems you add some trailing whitespace here.

@@ +198,5 @@
> +  // create a new array with the proper length and copy the
> +  // the valid data to it and return it.
> +  var newcards  = Array(current);
> +  for (i = 0; i < current; i++)
> +    newcards[i] = cards[i];

I am not sure what is this copying good for. Can you instead declare 'cards = []' above and .push() found cards into it?

Comment 8

[ISHIKAWA, Chiaki]

Attachment: card-is-null-fix.patch (take four)

Here is the latest version of the patch.
After carefully analyzing the session log and check the "... is undefined" errors,
I found I needed to fix a few places where
the return value of GetSelectedAbCards().
I decided to return null if there don't seem to be valid selections, but
then the caller sites needed to be modified to handle this explicit null return value.

TIA

PS: There was some indentation issues, but I sent this out since my understanding is that the release is near, and I want people to check for
the correctness of the patch sooner.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 786283 [details] [diff] [review]
card-is-null-fix.patch (take Three)

Canceling review on the old patch.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 801321 [details] [diff] [review]
card-is-null-fix.patch (take four)

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

Thanks for the patch! I have a few suggestions - mainly, I think we should fix up GetSelectedAbCards to always return an Array, but never allow that Array to contain nulls. Please see below.

-Mike

::: mail/components/addrbook/content/abContactsPanel.js
@@ +35,5 @@
>  
>  function addSelectedAddresses(recipientType)
>  {
>    var cards = GetSelectedAbCards();
> +  var count = !cards ? 0 : cards.length;

If !cards, how about we just return? There's no reason to stay in this function anymore.

::: mailnews/addrbook/content/abResultsPane.js
@@ +122,5 @@
>  
>    var mailingListCnt = 0;
>    var cardCnt = 0;
>    for (var i = 0; i < count; i++) {
> +    // there was a case where cards[i] was null.

With the plan I outline in my GetSelectedAbCards comment, this should no longer be necessary.

@@ +170,1 @@
>  function GetSelectedAbCards()

So this function returns null if no cards are selected? I don't think that makes a whole lot of sense. I think it makes more sense to return an empty Array in the event that no cards are selected - that way, we can assume that an Array will definitely be returned by this function, and we don't have to check for a non-null return value every time.

You'll need to update any points where the result of GetSelectedAbCards() is null checked.  A cursory check via mxr found these two:

https://mxr.mozilla.org/comm-central/source/mailnews/addrbook/content/abResultsPane.js#114
https://mxr.mozilla.org/comm-central/source/mail/components/addrbook/content/abCommon.js#393

These just need to be modified to assume that a (possibly empty) array is being passed.

@@ +174,5 @@
>    // if sidebar is open, and addressbook panel is open and focused,
>    // then use the ab view from sidebar (gCurFrame is from sidebarOverlay.js)
>    if (document.getElementById("sidebar-box")) {
>      const abPanelUrl =
> +          "chrome://messenger/content/addressbook/addressbook-panel.xul";

2 space identation here please.

@@ +180,3 @@
>          gCurFrame.getAttribute("src") == abPanelUrl &&
>          document.commandDispatcher.focusedWindow == gCurFrame.contentDocument.defaultView)
> +        abView = gCurFrame.contentDocument.defaultView.gAbView;

2 space indentation here too, please.

@@ +183,4 @@
>    }
>  
>    if (!abView)
> +      return null;

2 space indentation, and let's return []

@@ +193,5 @@
>    for (i = 0; i < count; ++i) {
>      var start = new Object;
>      var end = new Object;
>      abView.selection.getRangeAt(i,start,end);
> +    for (j = start.value; j <= end.value; ++j) {

This can be greatly simplified, if we assume that this function always returns an Array.

for (j = start.value; j <= end.value; ++j) {
  let card = abView.getCardFromRow(j);
  if (card) {
    cards.push(card);
  }
}

return cards;

Comment 11

[ISHIKAWA, Chiaki]

(In reply to Mike Conley (:mconley) from comment #10)
> Comment on attachment 801321 [details] [diff] [review]
> card-is-null-fix.patch (take four)
> 
> Review of attachment 801321 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch! I have a few suggestions - mainly, I think we should
> fix up GetSelectedAbCards to always return an Array, but never allow that
> Array to contain nulls. Please see below.
> 

Thank you for the quick comment. I will digest your comment and come up with a new patch, and then
run it through local |make mozmill| test and report back.

TIA

Comment 12

[ISHIKAWA, Chiaki]

Attachment: card-is-null-fix.patch (WIP)

I am very confused about what values are returned by low-level routines and
sprinkled dump statement.
Here are the summary of the output from the modified patches.
I culled the values from the session log recorded by script command under
linux and used
echo 
echo " ========================================"
echo " bugs related to cards is null "
echo " ========================================"

egrep "abView.selection.count" $1 | sort | uniq -c | sort -n -r
egrep "getCardFromRow" $1 | sort | uniq -c | sort -n -r
egrep "GetRangeAt" $1 | sort | uniq -c | sort -n -r

where $1 is the session file.


 ========================================
 bugs related to cards is null
 ========================================
    222 abView.selection.count = 0, abView.selection.getRangeCount() = 0
    139 abView.selection.count = 1, abView.selection.getRangeCount() = 1
     10 abView.selection.count = 3, abView.selection.getRangeCount() = 1
      9 abView.selection.count = 2, abView.selection.getRangeCount() = 1
     54 getCardFromRow(j) returned null: count, start.value, end.value, i, j = 1, 0, 0, 0, 0
    111 GetRangeAt: start.value = 0, end.value = 0
     22 GetRangeAt: start.value = 1, end.value = 1
     10 GetRangeAt: start.value = 0, end.value = 2
      9 GetRangeAt: start.value = 0, end.value = 1
      3 GetRangeAt: start.value = 3, end.value = 3
      3 GetRangeAt: start.value = 2, end.value = 2
 
The fifty four (54) cases are where the low-level getCardFromRow(j) returned null for non-obvious reasons: I am afraid there have been lower-level change, but could not home in. The code is not well commented and I gave up.
Anyway, about half the time, the loop is executed (count > 0),
getCardFromRow returns null, which seems a little suspect.

Anyway, other than this debug output, the patch seems to suppress the
warnings although I needed to check for the emptiness/nullptr of cards[0] now that the array is always returned and its first element may contain nullptr.

I will clean up the patch (WIP) and post later.
I am uploading this WIP patch and the output value if someone in the know can
use the values for guessing the root cause of the strange failure of
getCardFromRow(), etc.

TIA

Comment 13

[:aceman]

Comment on attachment 801321 [details] [diff] [review]
card-is-null-fix.patch (take four)

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

Yes, I would also like if we just returned an empty array. In that way callers can universally iterate over the return value and act on each member. If there are 0 members, that will not be a problem if operations are separate for each member. If they need to be handled as a group and do nothing if there are 0 members, than just using card.length instead of !card check is equally simple.

Comment 14

[ISHIKAWA, Chiaki]

Attachment: card-is-null-fix.patch (Take Five)

Here is the latest patch.

I tried to incorporate the comments. I hope my understanding was correct.
Now, after the initial modification of GetSelectedAbCards(), I needed to fix a few additional places which are in the patch.

Incidentally when I set verbose to 1, |make mozmill| test suite run produced the
following values:

 ========================================
 bugs related to cards is null
 ========================================
    228 abView.selection.count = 0, abView.selection.getRangeCount() = 0
    140 abView.selection.count = 1, abView.selection.getRangeCount() = 1
     10 abView.selection.count = 3, abView.selection.getRangeCount() = 1
      9 abView.selection.count = 2, abView.selection.getRangeCount() = 1
     55 Error: getCardFromRow(j) returned null: count, start.value, end.value, i, j = 1, 0, 0, 0, 0
    112 GetRangeAt: start.value = 0, end.value = 0, count = 1, i = 0
     22 GetRangeAt: start.value = 1, end.value = 1, count = 1, i = 0
     10 GetRangeAt: start.value = 0, end.value = 2, count = 1, i = 0
      9 GetRangeAt: start.value = 0, end.value = 1, count = 1, i = 0
      3 GetRangeAt: start.value = 3, end.value = 3, count = 1, i = 0
      3 GetRangeAt: start.value = 2, end.value = 2, count = 1, i = 0

I think mozilla code maintains a slightly broken data structure
when no selection is made (maybe aborted, maybe the address book is empty to begin with, etc.), and somehow the internal count is kept as one (which should have been zero), and confuse the routines here.

But anyway, the patch should make the JS code more robust against this kind of
intermittent errors

TIA

Comment 15

[:aceman]

Comment on attachment 802894 [details] [diff] [review]
card-is-null-fix.patch (Take Five)

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

I still do not understand some of the changes therefore I mark f- for now. But if I misunderstood any of mconley's wishes then you can ignore it.

Please also make the commit line (patch description better), e.g. "Bug 733535 - fix 'cards[i] is null' by making GetSelectedAbCards only return non-null cards"

::: mail/components/addrbook/content/abCommon.js
@@ +1,2 @@
> +/* -*- Mode: javascript; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 ; js-indent-level: -*- */
> +/* vim: set ts=2 et sw=2 tw=80: */

Is this the header found in other files too? If not, please do not add headers specific only to your environment.

@@ +405,4 @@
>  
> +  for (var i = 0; i < count; i++) {
> +    if(cards[i]) {
> +      var generatedAddress = GenerateAddressFromCard(cards[i]);

Try 'let' instead of 'var' for both variables.
Also, why do you check for "cards[i]" here? Is the passed in 'cards' the result of GetSelectedAbCards()? If yes, then it cards should not contain any null members after your fix there.

::: mail/components/addrbook/content/abContactsPanel.js
@@ +42,5 @@
> +
> +  if (!cards) {
> +    dump("addSelectedAddresses: cards was null. Should no happen.");
> +    return;
> +  }

!cards should not happen (after your change to GetSelectedAbCards), test for !cards.length.

@@ +49,5 @@
>  
>    for (var i = 0; i < count; i++) 
>    {
>      // turn each card into a properly formatted address 
> +    // Protect for cards[i] being nullptr

We are in JS, so this probably should say "null" instead of "nullptr".

@@ +50,5 @@
>    for (var i = 0; i < count; i++) 
>    {
>      // turn each card into a properly formatted address 
> +    // Protect for cards[i] being nullptr
> +    if(cards[i]) {

cards[i] can't be null after your fix if GetSelectedAbCards()...

@@ +52,5 @@
>      // turn each card into a properly formatted address 
> +    // Protect for cards[i] being nullptr
> +    if(cards[i]) {
> +      var address = GenerateAddressFromCard(cards[i]);
> +      if (address != "")

In abCommon.js this was testing for (!address), should we not make it the same here? And 'let' instead of 'var'. Also please remove the trailing whitespace in these lines.

::: mailnews/addrbook/content/abResultsPane.js
@@ +188,5 @@
>  
> +  if (verbose) 
> +    dump("abView.selection.count = " + abView.selection.count 
> +         + ", abView.selection.getRangeCount() = "
> +         + abView.selection.getRangeCount() + "\n");

To be removed in final version.

@@ +193,1 @@
>    var cards = new Array(abView.selection.count);

'cards = [];' here.
Otherwise you preallocate abView.selection.count, but only initialize less of them (if some values of abView.getCardFromRow(j) are null). And also u use .push into the array so I'd think actually the array will be extended so you end up with abView.selection.count+abView.selection.count members.

@@ +206,5 @@
> +           ", count = " + count + 
> +           ", i = " + i + "\n");
> +
> +    for (j = start.value; j <= end.value; ++j) {
> +      // avoid null assignment. GetRangeAt() may be buggy.

We do not know what is buggy here.
I'd make this comment say like 
"abView.getCardFromRow(j) may return null in some cases so in that case we do not want to add it to the array."

Comment 16

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #15)
> Comment on attachment 802894 [details] [diff] [review]
> card-is-null-fix.patch (Take Five)
>
> Review of attachment 802894 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> I still do not understand some of the changes therefore I mark f- for now.
> But if I misunderstood any of mconley's wishes then you can ignore it.

Thank you for your comments.  I am uploading a renewed patch
(work-in-progress) with additional dump to show you and interested
parties where strange values crept in before.  (Now I know that the
cause of strange values was that I missed the setting of |cards| to
|[]| as you suggested, and had extra slots, it seemed.)

Without setting |cards| to |[]|, I got the following summary from my
session log meaning that there are stray null entries that crept into
the return value, an array, of GetSelectedAbCards().

 ========================================
 bugs related to cards is null
 ========================================
    210 abView.selection.count = 0, abView.selection.getRangeCount() = 0
    135 abView.selection.count = 1, abView.selection.getRangeCount() = 1
     10 abView.selection.count = 3, abView.selection.getRangeCount() = 1
      9 abView.selection.count = 2, abView.selection.getRangeCount() = 1
(**The following line is still a legitimate error/strange value
returned by getCardFromRow().)
     53 Error: getCardFromRow(j) returned null: count, start.value, end.value, i, j = 1, 0, 0, 0, 0
    107 GetRangeAt: start.value = 0, end.value = 0, count = 1, i = 0
     22 GetRangeAt: start.value = 1, end.value = 1, count = 1, i = 0
     10 GetRangeAt: start.value = 0, end.value = 2, count = 1, i = 0
      9 GetRangeAt: start.value = 0, end.value = 1, count = 1, i = 0
      3 GetRangeAt: start.value = 3, end.value = 3, count = 1, i = 0
      3 GetRangeAt: start.value = 2, end.value = 2, count = 1, i = 0
->   54 Error: GetSelectedCardTypes: cards[i] was null. i = 0
->    9 Error: GetSelectedCardTypes: cards[i] was null. i = 1
->    6 Error: GetSelectedCardTypes: cards[i] was null. i = 2
->   25 Error: UpdateCardView: cards[0] was null
->   14 Error: abResultsController: selectedCard is null
->    1 Error: addSelectedAddresses: cards[i] was null. i = 0


The output was captured by running a shell script which essentially
did
echo " ========================================"
echo " bugs related to cards is null "
echo " ========================================"

egrep "abView.selection.count"	     $1 | sort | uniq -c | sort -n -r
egrep "getCardFromRow"		     $1 | sort | uniq -c | sort -n -r
egrep "GetRangeAt"		     $1 | sort | uniq -c | sort -n -r
egrep "Error: GetSelectedCardTypes"  $1 | sort | uniq -c | sort -n -r
egrep "Error: getCardFromRow(j)"     $1 | sort | uniq -c | sort -n -r
egrep "Error: UpdateCardView:"	     $1 | sort | uniq -c | sort -n -r
egrep "Error: abResultsController:"  $1 | sort | uniq -c | sort -n -r
egrep "Error: addSelectedAddresses:" $1 | sort | uniq -c | sort -n -r

where $1 is the session log file captured by linux's |script| command.

But with |cards| being set properly to |[]|, I got the following
summary from my session log.  No more stray null entries.

 ========================================
 bugs related to cards is null
 ========================================
    231 abView.selection.count = 0, abView.selection.getRangeCount() = 0
    137 abView.selection.count = 1, abView.selection.getRangeCount() = 1
     10 abView.selection.count = 3, abView.selection.getRangeCount() = 1
      9 abView.selection.count = 2, abView.selection.getRangeCount() = 1
     55 Error: getCardFromRow(j) returned null: count, start.value, end.value, i, j = 1, 0, 0, 0, 0
    109 GetRangeAt: start.value = 0, end.value = 0, count = 1, i = 0
     22 GetRangeAt: start.value = 1, end.value = 1, count = 1, i = 0
     10 GetRangeAt: start.value = 0, end.value = 2, count = 1, i = 0
      9 GetRangeAt: start.value = 0, end.value = 1, count = 1, i = 0
      3 GetRangeAt: start.value = 3, end.value = 3, count = 1, i = 0
      3 GetRangeAt: start.value = 2, end.value = 2, count = 1, i = 0


(Note: due to time-out issues, and some known
perma-orange errors such as popup doesn't show under linux sometimes,
the number of entries are different under two log sessions. This makes
exact comparison rather difficult. )

The uploaded patch incorporates this setting of |cards| to |[]|.

Now back to your original comment:

> Please also make the commit line (patch description better), e.g. "Bug
> 733535 - fix 'cards[i] is null' by making GetSelectedAbCards only return
> non-null cards"
>

Thank you. I picked up the message in the uploaded new patch.

> ::: mail/components/addrbook/content/abCommon.js
> @@ +1,2 @@
> > +/* -*- Mode: javascript; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 ; js-indent-level: -*- */
> > +/* vim: set ts=2 et sw=2 tw=80: */
>
> Is this the header found in other files too? If not, please do not add
> headers specific only to your environment.

This is an extension of existing mode lines found in many files:
MXR search shows this.
http://mxr.mozilla.org/comm-central/search?string=mode%3A+javascript%3B&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central
showed 199 similar entries.
E.g., mode: javascript;

Found 199 matching lines in 199 files
/calendar/base/src/calApplicationUtils.js (View Hg log or Hg annotations)
    line 1 -- /* -*- Mode: javascript; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 4 -*-
/calendar/base/src/calItipItem.js (View Hg log or Hg annotations)
    line 1 -- /* -*- Mode: javascript; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
/calendar/resources/content/calendarCreation.js (View Hg log or Hg annotations)
    line 1 -- /* -*- Mode: javascript; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 4 -*-
/mail/app/profile/all-thunderbird.js (View Hg log or Hg annotations)
    line 1 -- /* -*- Mode: JavaScript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

    ... ...

VIM option line follows the line in these files. (But I have not
tested VIM myself.)
I added js-indent-level. This makes it necessary to run javascript
mode in Emacs 24.3, the latest version, with desired amount of indentation.
Oops, I may have screwed up one mode line. '2' was missing in abCommon.js mode line. I have fixed it.

> @@ +405,4 @@
> >
> > +  for (var i = 0; i < count; i++) {
> > +	 if(cards[i]) {
> > +	   var generatedAddress = GenerateAddressFromCard(cards[i]);
>
> Try 'let' instead of 'var' for both variables.
> Also, why do you check for "cards[i]" here?

I have used let in the new patch (and also changed a few instances of
'var' near where the patch touched the code.)

>  Is the passed in 'cards' the
> result of GetSelectedAbCards()? If yes, then it cards should not contain any
> null members after your fix there.

Strange things do happen. But it was traced to my forgetting to
|cards| to |[]| as you suggested. But I am keeping the check in place.
For my rationale, see the comment at the end.

> ::: mail/components/addrbook/content/abContactsPanel.js
> @@ +42,5 @@
> > +
> > +  if (!cards) {
> > +	 dump("addSelectedAddresses: cards was null. Should no happen.");
> > +	 return;
> > +  }
>
> !cards should not happen (after your change to GetSelectedAbCards), test for
> !cards.length.
>

I now check for !cards.length, but just in case, I left the check for
cards: defensive programming to prepare for future changes.
Again, see my rationale at the end.

> @@ +49,5 @@
> >
> >    for (var i = 0; i < count; i++)
> >    {
> >	 // turn each card into a properly formatted address
> > +	 // Protect for cards[i] being nullptr
>
> We are in JS, so this probably should say "null" instead of "nullptr".

done.

>
> @@ +50,5 @@
> >    for (var i = 0; i < count; i++)
> >    {
> >	 // turn each card into a properly formatted address
> > +	 // Protect for cards[i] being nullptr
> > +	 if(cards[i]) {
>
> cards[i] can't be null after your fix if GetSelectedAbCards()...

Agreed, but again see my rationale at the end.

>
> @@ +52,5 @@
> >	 // turn each card into a properly formatted address
> > +	 // Protect for cards[i] being nullptr
> > +	 if(cards[i]) {
> > +	   var address = GenerateAddressFromCard(cards[i]);
> > +	   if (address != "")
>
> In abCommon.js this was testing for (!address), should we not make it the
> same here? And 'let' instead of 'var'. Also please remove the trailing
> whitespace in these lines.

Checking for !address may not be quite correct. I think we need to
check for "" as well. (Not sure if value other than "" is thrown under 
error conditions.) Frankly, I feel that we can use better comments in
source files here (or even bad comments if ANY AT ALL).

 - I used 'let' again here.
 - I used delete-trailing-whitespace of emacs and that
  removed a few more extra blanks.

>
> ::: mailnews/addrbook/content/abResultsPane.js
> @@ +188,5 @@
> >
> > +  if (verbose)
> > +	 dump("abView.selection.count = " + abView.selection.count
> > +	      + ", abView.selection.getRangeCount() = "
> > +	      + abView.selection.getRangeCount() + "\n");
>
> To be removed in final version.

Yeah, but with the bug hitting us this summer, I am tempted to leave
them in. Won't the JS compiler do the simple optimization of
eliminating never executed |if (0) {...}| block when it byte-compiles
JS code?  And again, see my rationale for keeping some debugging code
in place explained at the end. When we set verbose to 0, the output from these
lines won't be printed.

>
> @@ +193,1 @@
> >    var cards = new Array(abView.selection.count);
>
> 'cards = [];' here.
> Otherwise you preallocate abView.selection.count, but only initialize less
> of them (if some values of abView.getCardFromRow(j) are null).

Aha, I missed this in my earlier patches, and it may explain the
strange null references which I had to protect against.

> And also u
> use .push into the array so I'd think actually the array will be extended so
> you end up with abView.selection.count+abView.selection.count members.

Right. This seemed to be the problem of some run-time errors.

> @@ +206,5 @@
> > +		", count = " + count +
> > +		", i = " + i + "\n");
> > +
> > +	 for (j = start.value; j <= end.value; ++j) {
> > +	   // avoid null assignment. GetRangeAt() may be buggy.
>
> We do not know what is buggy here.
> I'd make this comment say like
> "abView.getCardFromRow(j) may return null in some cases so in that case we
> do not want to add it to the array."

done.

Now, I think the patch is logically correct  more or less.

Here is a comment about extra checks.

My rationale to keep some seemingly extra check in place:

Well, to start off, there is NO written comments about the specs of
these functions, etc. A well-meaning programmer may change the
semantics without announcement breaking many functions. I think this
happened over this spring and summer.

I am sure it will happen again with the manner TB code is managed
now. More likely since there is no well documented spec now and people
who are familiar with the original code is drifting off slowly now
that TB is a user community project.  With this background, I am
inclined to keep the extra checks in place: defensive programming in
principle, and addressing the issue of missing comment by offering
an executable spec., so to speak.


BTW, of course, some of you will mention that they can be left out and kept
in the bugzilla.

I disagree here.

From my reading of very scantily commented JS code,
we should keep as much semantic information in JS code (especially
thunderbird portion now that it is community project).

Frankly, I don't bother to read up the hg blame and other patches
unless I am really forced in order to learn the unwritten assumptions
about the parameter values and return type and value ranges and such.
They REALLY should be commented in the source code.
If there are simple explanations in the form of comments in the source files, I don't have check
the hg blame and other tools at all.
(Focusing on the change that introduced the bug or misbehavior, 
one is asked to run hg bisect.
hg bisect doesn't cut mustard for me. TB source tree contains
two independent hg repositories, one for TB main portion and
the other, a copy of mozilla-central with minor tweaks for TB.
Also, compiling TB takes longer, it seems, than mozilla firefox, and
so I have never bothered to run |hg bisect| on my slooow PC. Compiling alone can
take hours for a single configuration file change :-( )

To recap our situation, 
here, we are changing the semantics of GetSelectedAbCards() slightly
in order to make the formerly working code work again. It seems to
have been broken by some lower-level routines somewhere
(selection-related code) this spring or summer.

But since the underlying assumptions or meaning of arguments are
hardly commented (I would say, not commented at all), we can't really
see how the code would behave or SHOULD behave, etc.

With such a situation, I want to keep fixing the source files 
by placing sanity checks that will keep the code sane by refusing to 
process arguments outside its working domain. 
(These bad values are thrown by changed code elsewhere, or
by our modifications even. Our understanding of complex TB doesn't seem to be through nowadays.)
These checks make explicit the assumption and
the limit of the code in the source file. From the patch in this
state, someone can take time to write spec-like document, er,
paragraphs that can be embedded as comment.

My point is this: whoever changed the lower-level routine this spring
or early summer may do it again in a manner that affects the code
discussed here.	 I would like to make this code more robust to such
changes.

To be frank, I would love to see such checks remain in the source code
file itself, rather than in bugzilla. It will not be quite easy to
find out the rational of a strange code line. I tried a few chasing
myself using bugzilla this past several months, but eventually they
were not that useful at all. Not sure if this was a result of
fundamental deficiency (having two independent repository doesn't
help), or that the particular bugs, and particular patches and/or
changesets were not commented very well, etc. (Also, often the code in
question is imported long time ago and I ended up this wall of hg1.0
or some such nomenclature which shows that the code was imported from
CVS long time ago, etc., and no history is kept.   But why not leaving an
important note about parameter values, return values and limitations
in the source code itself. I find the explicit mentions of some issues
and reference numbers of the bugzilla entry in the source code very useful.

But, of course, it is module owner's choice eventually.

Right, we should probably write down the javadoc style comments to
keep track of the changes *we* have made to the parameters and assumptions.
Anyone? :-)

TIA

Comment 17

[ISHIKAWA, Chiaki]

Attachment: card-is-null-fix.patch (Take Six)

Here is a modified patch again to incorporate aceman's comments and one
valid bug fix.

TIA

Comment 18

[:aceman]

Comment on attachment 803184 [details] [diff] [review]
card-is-null-fix.patch (Take Six)

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

I haven't run it (as my mozmill is broken), but the logic seems fine now. mconley will decide if he wants so much output in case of error. But yes, after this patch a null card will really be unexpected. But I think we added comments into the function var GetSelectedAbCards so I hope anybody changing it in the future will see that and not change the function to suddenly return nulls again. I think the checks at all the callers are a bit superfluous in this light, but mconley will decide.

::: mail/components/addrbook/content/abCommon.js
@@ +376,5 @@
>      var directory = gDirectoryTreeView.getDirectoryAtIndex(gDirTree.currentIndex);
>      if (directory.isMailList) {
>        var listCardsCount = directory.addressLists.length;
>        var cards = new Array(listCardsCount);
> +      for (let i = 0; i < listCardsCount; ++i)

In this version of the patch there is no change in this loop so just changing var to let is strange.

@@ +393,5 @@
>    var addresses = "";
>  
> +  // The original code assumed that when count was positive,
> +  // cards[0] could not be nullptr, but the situation has changed.
> +  // We must protect for cards[0] being null.

Did this function actually need any change?

::: mail/components/addrbook/content/abContactsPanel.js
@@ +44,5 @@
> +    dump("addSelectedAddresses: cards was null or cards.length was zero. Should not happen.\n");
> +    return;
> +  }
> +
> +  count = cards.length;

let count = cards.length;

@@ +51,4 @@
>    {
> +    // turn each card into a properly formatted address
> +    // Protect for cards[i] being null
> +    if(cards[i]) {

Space after if .

@@ +137,5 @@
>    // double click for ab panel means "send mail to this person / list"
>    AbNewMessage();
>  }
>  
> +function UpdateCardView()

Just cleaning up whitespace is usually not wanted. Never let the editor clean the whole file automatically. Changes of whitespace when the line would otherwise not be logically changed clutters a patch unnecessarily. But mconley will decide.

::: mail/components/addrbook/content/addressbook.js
@@ +645,5 @@
>  function AbIMSelected()
>  {
>    let cards = GetSelectedAbCards();
>  
> +  if(!cards || cards.legnth != 1) {

Space after if .

Comment 19

[ISHIKAWA, Chiaki]

Attachment: card-is-null-fix.patch (Take Seven)

This is the latest patch.
I tried to incorporate the stylistic changes (a space after if: but then found other instances of such ill-formed lines and fixed them as well on the side).
Tried to put an explanatory comment. 

TIA

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 812976 [details] [diff] [review]
card-is-null-fix.patch (Take Seven)

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

The approach is sane, but there's a ton of debugging stuff that needs to be removed / cleaned up. I've mentioned it below. Thanks!

::: mail/components/addrbook/content/abCommon.js
@@ +392,5 @@
>  {
>    var addresses = "";
>  
> +  if (!cards) {
> +    dump("ERROR: GetAddressesForCards: |cards| is null. Should not happen.\n");

Please remove all dump statements from your patch. If we really need to send out an error message, we should do it with Components.utils.reportError.

@@ +397,4 @@
>      return addresses;
> +  }
> +
> +  // The original code assumed that when count was positive,

I don't think this comment adds much in the way of value.

@@ +406,2 @@
>  
> +  // There was a time when cards[i] was null, but

I don't think this comment should be here - instead, let's put some documentation on the function itself, saying that we do not handle the case where there is one or more null-ish element in the Array.

@@ +406,5 @@
>  
> +  // There was a time when cards[i] was null, but
> +  // now it is fixed. Always non-null element is pushed
> +  // into cards[] array.
> +  for (var i = 0; i < count; i++) {

I think we can simplify all of this:

let generatedAddresses = cards.map(GenerateAddressFromCard)
                              .filter(function(aAddress) {
  return aAddress;
});
return generatedAddresses.join(',');

::: mail/components/addrbook/content/abContactsPanel.js
@@ +39,5 @@
>  {
>    var cards = GetSelectedAbCards();
> +
> +  if (!cards) {
> +    dump("ERROR: addSelectedAddresses: |cards| was null. Should not happen.");

No dumps please. Components.utils.reportError or nothing. :)

@@ +50,5 @@
>    {
> +
> +    // turn each card into a properly formatted address.
> +    // We no longer have to protect for cards[i] being null after the
> +    // change of GetSelectedAbCards()

"after the change of GetSelectedAbCards" is pretty ambiguous. How about:

"We can assume that none of the cards returned from GetSelectedAbCards() are null"

::: mail/components/addrbook/content/addressbook.js
@@ +286,5 @@
>  {
>    var cards = GetSelectedAbCards();
>  
> +  if (!cards) {
> +    dump("ERROR: UpdateCardView: |cards| is null. Should not happen.");

Again, no dumps. I'll stop mentioning this now.

@@ +459,5 @@
>  
>      gStatusText.setAttribute("label", statusText);
>    }
>    catch(ex) {
> +    dump("ERROR: failed to set status text:  " + ex + "\n");

Oof - let's use Components.utils.reportError for this.

@@ +656,3 @@
>    if (cards.length != 1) {
>      Components.utils.reportError("AbIMSelected should only be called when 1"
> +                                 + " card is selected. There are " + (cards ? cards.length : 0)

If !cards, we never would have gotten here (we'd have returned on line 653), so I don't think this ternary adds anything.

@@ +664,5 @@
>  
> +  // Note that cards[0] could be null due to the changes in the lower-level routines.
> +  // This should NOT happen any more, but a sanity check is in place.
> +  if (!card) {
> +    dump("ERROR: AbIMSelected: one card was selected, but its only member was null.)");

No dump please.

::: mailnews/addrbook/content/abResultsPane.js
@@ +112,5 @@
>  
>  function GetSelectedCardTypes()
>  {
>    var cards = GetSelectedAbCards();
> +  if (!cards) {                 // sanity check

No need for the comment here.

@@ +113,5 @@
>  function GetSelectedCardTypes()
>  {
>    var cards = GetSelectedAbCards();
> +  if (!cards) {                 // sanity check
> +    dump ("ERROR: GetSelectedCardTypes:should not happen. |cards| is null. n");

No dump please.

@@ +125,5 @@
>    var cardCnt = 0;
>    for (var i = 0; i < count; i++) {
> +    // There were cases where cards[i] was null.  So needed to check
> +    // this explicitly. But no longer necessary after the change of
> +    // GetSelectedAbCards()

Like before, this is ambiguous. It's enough to say that we can assume no values from GetSelectedAbCards will be null.

@@ +175,4 @@
>  function GetSelectedAbCards()
>  {
>    var abView = gAbView;
> +  var verbose = 0;              // to control debug output. 

This debugging stuff definitely needs removal before this can land.

Comment 21

[ISHIKAWA, Chiaki]

Attachment: Proposed patch (take 8).

Hi,

Sorry for late update. I incorporated all your comments somehow.

I ran |make mozmill| a few times with the patch and did not see card[i] is null message any more... (OTOH, such a case, if it ever happens, is now checked explicitly, and a warning/error is printed by reportError() to debug console).

TIA

Comment 22

[Magnus Melin [:mkmelin]]

Comment on attachment 8360798 [details] [diff] [review]
Proposed patch (take 8).

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

Driveby.

As a general comment. I understand you want to document things in the process of fixing the problem, but the kind of comments i've pointed out below would lead to confusion later and are helpful *only* during debugging. They should generally not go into the main source.

::: mail/components/addrbook/content/abCommon.js
@@ +390,5 @@
> +// cards.
> +
> +//  The code assumes cards[0] could not be null, and we make sure that
> +//  this is so by GetSelectedAbCards() in
> +//  mailnews/addrbook/content/abResultsPane.js.

Remove this comment.

@@ +396,5 @@
>  {
>    var addresses = "";
>  
> +  if (!cards) {
> +    Components.utils.reportError("GetAddressesForCards: |cards| is null. Should not happen.");

Remove "Should not happen". That is pretty obvious if you log it as an error in the console

@@ +409,1 @@
>  

false, "", null, undefined, 0 are called falsy

::: mail/components/addrbook/content/abContactsPanel.js
@@ +40,5 @@
>    var cards = GetSelectedAbCards();
> +
> +  if (!cards) {
> +    return;
> +  }

no need for braces for the single-line if

@@ +47,5 @@
>  
>    for (var i = 0; i < count; i++) 
>    {
> +
> +    // turn each card into a properly formatted address.

Capitalize Turn

::: mail/components/addrbook/content/addressbook.js
@@ +294,4 @@
>    // display the selected card, if exactly one card is selected.
>    // either no cards, or more than one card is selected, clear the pane.
> +  // Beware cards[0] could be NULL before the change to the semantics of
> +  // GetSelectedAbCards() due to changes in the underlying code.

This comment should also be removed.

I'm not even sure what it's referring to ATM, but after a few code changes it's definitely not something anyone would find understand.

@@ +647,5 @@
>  {
>    let cards = GetSelectedAbCards();
>  
> +  if (!cards) {
> +    Components.utils.reportError("ERROR: AbIMSelected: |cards| is null. Should not happen.");

it's already reported as an error in the console. so
"GetSelectedAbCards: cards is null" is sufficient, if we do want to bother reporting it

@@ +648,5 @@
>    let cards = GetSelectedAbCards();
>  
> +  if (!cards) {
> +    Components.utils.reportError("ERROR: AbIMSelected: |cards| is null. Should not happen.");
> +    return; 

trailing whitespace

@@ +655,4 @@
>    if (cards.length != 1) {
>      Components.utils.reportError("AbIMSelected should only be called when 1"
> +                                 + " card is selected. There are " 
> +                                 + cards.length 

trailing whitespaces
and plusses should be on the end of the lines

@@ +666,5 @@
> +  // This should NOT happen any more, but a sanity check is in place.
> +  if (!card) {
> +    Components.utils.reportError("AbIMSelected: one card was selected, but its only member was null.");
> +    return;
> +  }

i don't this is needed either. The nice thing with javascript is such errors aren't fatal to the application, the sanity check is already there as it would result in an error in the console, when someone tries to access the card

::: mailnews/addrbook/content/abResultsPane.js
@@ +112,5 @@
>  
>  function GetSelectedCardTypes()
>  {
>    var cards = GetSelectedAbCards();
> +  if (!cards) {                 

trailing whitspace

@@ +113,5 @@
>  function GetSelectedCardTypes()
>  {
>    var cards = GetSelectedAbCards();
> +  if (!cards) {                 
> +    Components.utils.reportError("ERROR: GetSelectedCardTypes:should not happen. |cards| is null.");

misleading error message. not that i think we need to report an error either way

@@ +130,5 @@
>      else
>        cardCnt++;
>    }
> +  // Sanity check.
> +  if(cardCnt == 0 && mailingListCnt == 0)

add space after if

@@ +168,5 @@
> +//
> +// Return a list always (empty list possibly).
> +//
> +// No longer pushes null/empty element into the returned list.
> +//

function style documentation is 
/**
 * Return a (possibly empty) list of selected cards.
 */

@@ +190,2 @@
>  
> +  var cards = new Array(0); // abView.selection.count;

let cards = [];

@@ +192,1 @@
>    var i, j;

while here, please move the declarations to the for loop where they are used. no need to declare these earlier

@@ +210,5 @@
> +	       start.value + ", " +
> +	       end.value + ", " +
> +	       i + ", " + j );
> +	}
> +      }

not fond of adding this either. even if it were to happen, there's very little we could do to figure out why (that we couldn't already)

Comment 23

[ISHIKAWA, Chiaki]

Attachment: Proposed patch (take 9)

Thank you for the review and comment.

(In reply to Magnus Melin from comment #22)
> Comment on attachment 8360798 [details] [diff] [review]
> Proposed patch (take 8).
>
> Review of attachment 8360798 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Driveby.
>
> As a general comment. I understand you want to document things in the
> process of fixing the problem, but the kind of comments i've pointed out
> below would lead to confusion later and are helpful *only* during debugging.
> They should generally not go into the main source.

Let me modify the patch from this view point. See below.

> ::: mail/components/addrbook/content/abCommon.js
> @@ +390,5 @@
  ...
> Remove this comment.

done

> @@ +396,5 @@
  ...
> > +  if (!cards) {
> > +	 Components.utils.reportError("GetAddressesForCards: |cards| is null. Should not happen.");
>
> Remove "Should not happen". That is pretty obvious if you log it as an error
> in the console

Done.

> @@ +409,1 @@
> >
>
> false, "", null, undefined, 0 are called falsy

???

> ::: mail/components/addrbook/content/abContactsPanel.js
  ...
> > +  if (!cards) {
> > +	 return;
> > +  }
>
> no need for braces for the single-line if

Done.

But I noticed that there is a discussion about in devel-platform mailing list
about uniformly adding	"{", and "}" even in the case there is only
a line after if (logical expression).

I have to concur with somebody who mentioned that when one wants to
add additional step (like debug dump) in such a situation, one is
required to manually add "{", "}" in anyway.

E.g. :
   if (logical)
      return rv;

===>

   if (logical) {
     ...do_something...
     return rv;
   }

But I digress.

> @@ +47,5 @@
  ...
> > +
> > +	 // turn each card into a properly formatted address.
>
> Capitalize Turn

Done

> ::: mail/components/addrbook/content/addressbook.js
> @@ +294,4 @@
  ...
> > +  // Beware cards[0] could be NULL before the change to the semantics of
> > +  // GetSelectedAbCards() due to changes in the underlying code.
>
> This comment should also be removed.
>
> I'm not even sure what it's referring to ATM, but after a few code changes
> it's definitely not something anyone would find understand.

Done.

> @@ +647,5 @@
  ...
> > +  if (!cards) {
> > +	 Components.utils.reportError("ERROR: AbIMSelected: |cards| is null. Should not happen.");
>
> it's already reported as an error in the console. so
> "GetSelectedAbCards: cards is null" is sufficient, if we do want to bother
> reporting it

Done.

> @@ +648,5 @@
  ...
> > +  if (!cards) {
> > +	 Components.utils.reportError("ERROR: AbIMSelected: |cards| is null. Should not happen.");
> > +	 return;
>
> trailing whitespace

Done.
At the same time, in this patch, I took the liberty of fixing some
other trailing spaces which I did not notice before.

> @@ +655,4 @@
> >    if (cards.length != 1) {
> >	 Components.utils.reportError("AbIMSelected should only be called when 1"
> > +				      + " card is selected. There are "
> > +				      + cards.length
>
> trailing whitespaces
> and plusses should be on the end of the lines

Done. THe position of "+" is one thing that confuses me (coming from GNU coding style).

> @@ +666,5 @@
> > +  // This should NOT happen any more, but a sanity check is in place.
> > +  if (!card) {
> > +	 Components.utils.reportError("AbIMSelected: one card was selected, but its only member was null.");
> > +	 return;
> > +  }
>
> i don't this is needed either. The nice thing with javascript is such errors
> aren't fatal to the application, the sanity check is already there as it
> would result in an error in the console, when someone tries to access the
> card

Here, I would like to leave the message as is.

REASON: The operation in this javascript file began failing last year
around early Spring.  Nobody knows for sure, but a few suspected the
change in the underlying code that handles the selection in a list,
etc., but TB community is short of man-power and nobody could get down
to the bottom of the issue.  

Instead, we did a bandage job of fixing
the problem at this layer instead of going down into m-c portion of
the code (is my take on the situation).

Problem is that we do not know the exact cause and so there is no
guarantee that the code here is expected to work in the next several
months until the current snapshot (so to speak) will hit ESR.

Because of this, I would rather print such a message just in case the
code breaks again. (even if it is in debug console which an ordinary
user may not look often) instead of getting a mysterious "card is
null" or some message from the JavaScript interpreter. (I mean, to be
honest, to the future debuggers, I would think "should not happen"
will be an indication of the buggy state, but as long as long-time
veteran of code maintenance is there, I suppose "card is null" would
be enough.)

I have been stumped at the warning/error messages in |make mozmill|
log: many say something which would have meant something to the
original developer, but not quite indicative of what is wrong with it
to newcomer to the debugging efforts. 

Yes, right. I should be careful not to repeat these mistakes and should print something that would be meaningful to future would-be debuggers/developers.

Instead of leaving the check staying here, like you say, we can omit 
the check for |!card| completely, and let JS interpreter prints messages to error log during |make mozmill|.

I don't know which is better. It is a matter of whether being able to
figure out that something is wrong in a nicer way once TB binary may
start to act in a strange manner on user PC at beta version stage,
maybe. I hope the breakage won't happen suddenly once ESR is released,
but who knows :-(
If the cause of the bug is in the changed behavior of underlying code,
we should be hit with a bug BEFORE ESR, I hope, but again who knows?

> ::: mailnews/addrbook/content/abResultsPane.js
> @@ +112,5 @@
  ...
> > +  if (!cards) {
>
> trailing whitspace

Done

> @@ +113,5 @@
  ...
> > +  if (!cards) {
> > +	 Components.utils.reportError("ERROR: GetSelectedCardTypes:should not happen. |cards| is null.");
>
> misleading error message. not that i think we need to report an error either
> way

I left it as is according to my reasoning above.
I wonder what others think.

> @@ +130,5 @@
  ...
> > +  // Sanity check.
> > +  if(cardCnt == 0 && mailingListCnt == 0)
>
> add space after if

Done. BTW, I also noticed a few places where blank was missing and fixed them in my patch.

> @@ +168,5 @@
> > +//
> > +// Return a list always (empty list possibly).
> > +//
> > +// No longer pushes null/empty element into the returned list.
> > +//
>
> function style documentation is
> /**
>  * Return a (possibly empty) list of selected cards.
>  */

Done.

> @@ +190,2 @@
> >
> > +  var cards = new Array(0); // abView.selection.count;
>
> let cards = [];

Done. (Sorry I did not remove the comment by mistake.)

> @@ +192,1 @@
> >    var i, j;
>
> while here, please move the declarations to the for loop where they are
> used. no need to declare these earlier

Done. I am not a javascript programmer and so was not sure of what the
difference between |var| and |let| even. Well, actually, I checked and
understood that |let| has a block scope. So maybe |let| would have been better.
I will modify them in the next cycle.

> @@ +210,5 @@
> > +	       start.value + ", " +
> > +	       end.value + ", " +
> > +	       i + ", " + j );
> > +	}
> > +	   }
>
> not fond of adding this either. even if it were to happen, there's very
> little we could do to figure out why (that we couldn't already)

My point of leaving this is that, if the buggy behavior manifests on
user PC, then at least these numbers would give a little clue that there is at the moment  to would-be debugger(s).  (Of course, we don't know much about the
root cause.)  

I was hoping that a would-be debugger/developer from the
man-power-starved TB community may step in after seeing the error in
the RELEASED version, and may be able to use this information as a
starting clue.

Generally speaking, I am for the dumping of as much as internal state
and such for debugging purposes of released software. (I have noticed
that many bugzill entries did not have such information for analysis
and thus withered there unattended. That is one reason. "Please set this environment variable ..., and send back the log file, may not be enough for
many rare bugs. These bugs may be rare, but if crashing TB wipes out a
message being written, that is a disaster for a user: happened to me a few times. So I am now conditioned to write a lengthy message in Emacs and then paste it into TB. Shudder.)

But I agree that whether to leave this in or not
 is a close call in terms of this bug.  If we are
confident that the defensive programming in this javascript now helps
us avoiding the buggy behavior in the released version, then I have no
qualm about deleting the above dump.  

But I am NOT THAT confident.

Right now, TB code is at the whim of any changes in the undocumented
subtle behavioral change from the underlying code. That is the cause
of my uneasiness.  

All we can do is shake out the problems of impedence mismatch (so to speak) 
between upper level TB code and underlying core code by running tests before release.

Well, my observation is this: I thought we could eliminate more or
less all the frivolous JavaScript errors in C-C last mid-summer and was
quite happy. But then I realized new such warnings/errors have kept
marching in again from the changes in underlying code in m-c portion.
Probably TB code could not stand still even if no new features were introduced (!).
But, as　of now, TB community does not seem to have enough man-power to keep up
with even such mundane changes.

Case in point.

These are the strict warnings I got from |make mozmill| runs from local DEBUG BUILD of TB lately.
The number preceding each line is the frequency of occurences.
(I have filtered out warnings relatd to jquery.js:somehow jquery.tmp.js have crept in below.)

First strict checking warning from JavaScript:

     55 JavaScript strict warning: chrome://messenger/content/tabmail.xml, line 352: reference to undefined property aTabType.panelId
     55 JavaScript strict warning: chrome://global/content/bindings/tree.xml, line 50: reference to undefined property this.treeBoxObject.columns
     55 JavaScript strict warning: chrome://global/content/bindings/findbar.xml, line 262: reference to undefined property this._browser.finder
     35 JavaScript error: chrome://global/content/bindings/findbar.xml, line 262: this._browser.finder is undefined
     12 JavaScript strict warning: chrome://messenger/content/newmailaccount/jquery.tmpl.js, line 123: reference to undefined property def[1]
     11 JavaScript strict warning: chrome://global/content/bindings/tree.xml, line 53: reference to undefined property this.treeBoxObject.view
      8 JavaScript error: chrome://messenger/content/cloudfile/addAccountDialog.js, line 198: this._accountType is null
      4 JavaScript error: chrome://messenger/content/cloudfile/addAccountDialog.js, line 118: this._settings.contentDocument.body is undefined
      3 JavaScript strict warning: chrome://messenger/content/nsContextMenu.js, line 102: reference to undefined property this.onEditableArea
      2 JavaScript strict warning: file:///new-hd1/extra/ishikawa/TB-3HG/objdir-tb3/mozilla/dist/bin/components/storage-mozStorage.js, line 229: assignment to undeclared variable encType
      2 JavaScript error: chrome://messenger/content/folderDisplay.js, line 2365: this.view.dbView is null
      1 JavaScript strict warning: resource:///modules/dbViewWrapper.js, line 1046: reference to undefined property this._underlyingFolders[0]
      1 JavaScript strict warning: chrome://messenger/content/msgMail3PaneWindow.js, line 1041: reference to undefined property treeBoxObj.view


Here is a set of errors that are thrown all the way to the top of
JavaScript intepreter without getting caught.

      7 [Exception... "'[JavaScript Error: "this.view.displayedFolder is null" {file: "chrome://messenger/content/folderDisplay.js" line: 1072}]' when calling method: [nsIMsgSearchNotify::onSearchDone]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"	 location: "JS frame :: resource:///modules/searchSpec.js :: SearchSpec_dissociateView :: line 141"  data: yes]
      6 [Exception... "'[JavaScript Error: "aMsgHdr is null" {file: "chrome://messenger/content/mailWindowOverlay.js" line: 2832}]' when calling method: [nsIMsgHeaderSink::onEndMsgHeaders]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]
      3 [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIMsgMailNewsUrl.server]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: resource:///modules/activity/alertHook.js :: alertHook.onAlert :: line 48"  data: no]
      1 [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIMsgDBView.getMsgHdrsForSelection]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://messenger/content/folderDisplay.js :: FolderDisplayWidget.prototype.selectedMessages :: line 2029"	 data: no]
      1 [Exception... "'[JavaScript Error: "this.folderDisplay.view.dbView is null" {file: "chrome://messenger/content/messageWindow.js" line: 252}]' when calling method: [nsIMsgDBViewCommandUpdater::summarizeSelection]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: resource:///modules/dbViewWrapper.js :: DBViewWrapper__createView :: line 1047"  data: yes]
      1 [Exception... "'[JavaScript Error: "this._mostRecentActivityForFolder[aFolder.URI] is undefined" {file: "resource:///modules/activity/pop3Download.js" line: 93}]' when calling method: [nsIPop3ServiceListener::onDownloadCompleted]"	nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: resource://mozmill/modules/frame.js -> file:///new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mail/test/mozmill/instrumentation/test-instrument-setup.js :: remove_account :: line 93"  data: yes]
      1 [Exception... "'[JavaScript Error: "document.getElementById(...) is null" {file: "chrome://messenger/content/chat/imStatusSelector.js" line: 36}]' when calling method: [nsIObserver::observe]"	 nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: resource:///modules/chatHandler.jsm :: ChatCore.init/<.onPromptStart :: line 74"  data: yes]
      1 [Exception... "'[JavaScript Error: "_smtpServerAdded is not defined" {file: "resource:///modules/mailInstrumentation.js" line: 68}]' when calling method: [nsIObserver::observe]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"	location: "JS frame :: resource://mozmill/modules/frame.js -> file:///new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mail/test/mozmill/instrumentation/test-instrument-setup.js :: remove_account :: line 95"  data: yes]
      1 [Exception... "'[JavaScript Error: "_smtpServerAdded is not defined" {file: "resource:///modules/mailInstrumentation.js" line: 68}]' when calling method: [nsIObserver::observe]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"	location: "JS frame :: chrome://messenger/content/accountcreation/createInBackend.js :: createAccountInBackend :: line 93"  data: yes]

(For some reason, |make mozill| claims that it has passed most of the test: one reason seems to be that regular expression to catch errors in |make mozmill| does not seem to bother checking these. )

At one time last summer, the count went about five or so each (strict
error, and exceptions from JavaScript), and I was so happy to see the
small number.  But the numbers went up again lately.  (Granted, that
some are in the error handling portion of account setup or something:
it never seems to work on my local PC although it seems to work in TB
TryServer. But others are valid concern.)

It is an uphill battle just to keep these under control with TB c-c
source code, and I feel like any help to the would-be future
debugger/developer should be there in the source code.
(Yes, I agree that the comment /
message should be very clear and I thank you for the suggestions.)

Any thoughts from other passers-by?

TIA

Comment 24

[Magnus Melin [:mkmelin]]

(In reply to ISHIKAWA, Chiaki from comment #23)
> > false, "", null, undefined, 0 are called falsy
> 
> ???

What you referred to as null-ish is called falsy


> Yes, right. I should be careful not to repeat these mistakes and should
> print something that would be meaningful to future would-be
> debuggers/developers.
> 
> Instead of leaving the check staying here, like you say, we can omit 
> the check for |!card| completely, and let JS interpreter prints messages to
> error log during |make mozmill|.

In general if things "really shouldn't happen" then I think it's best to just let it blow up. Code that hides very wrong/unexpected states often just cause more problems later on.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8362409 [details] [diff] [review]
Proposed patch (take 9)

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

I think this looks pretty good! Just some nits to clean up. Also, are you ensuring that the addressbook tests are still passing?

::: mail/components/addrbook/content/addressbook.js
@@ +781,5 @@
>          let selected = GetSelectedAbCards();
>  
>          if (selected.length != 1)
>            return false;
> + 

Nit - trailing ws.

::: mailnews/addrbook/content/abResultsPane.js
@@ +130,5 @@
>      else
>        cardCnt++;
>    }
> +  // Sanity check.
> +  if (cardCnt == 0 && mailingListCnt == 0)

The only way this could possibly be true is if count was 0 - but if count was 0, we'd never have gotten here. I think we can go ahead and remove this check.

@@ +167,5 @@
>  
> +/**
> + * Return a (possibly empty) list of cards
> + *
> + * No longer pushes null/empty element into the returned list.

I don't think we should document what this function _no longer_ does, but what it _currently_ does.

@@ +190,2 @@
>  
> +  var cards = [];

Use let, not var - that applies to all new variables.

@@ +202,5 @@
> +      var tmp = abView.getCardFromRow(j);
> +      if (tmp) {
> +	cards.push(tmp);
> +      } else {
> +        // very strange error!

I don't think this comment adds much that the error string passed to reportError doesn't already provide.

Comment 26

[ISHIKAWA, Chiaki]

(In reply to Mike Conley (:mconley) from comment #25)
> Comment on attachment 8362409 [details] [diff] [review]
> Proposed patch (take 9)
> 
> Review of attachment 8362409 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this looks pretty good! Just some nits to clean up. Also, are you
> ensuring that the addressbook tests are still passing?
> 
> ::: mail/components/addrbook/content/addressbook.js
> @@ +781,5 @@
> >          let selected = GetSelectedAbCards();
> >  
> >          if (selected.length != 1)
> >            return false;
> > + 
> 
> Nit - trailing ws.
> 
> ::: mailnews/addrbook/content/abResultsPane.js
> @@ +130,5 @@
> >      else
> >        cardCnt++;
> >    }
> > +  // Sanity check.
> > +  if (cardCnt == 0 && mailingListCnt == 0)
> 
> The only way this could possibly be true is if count was 0 - but if count
> was 0, we'd never have gotten here. I think we can go ahead and remove this
> check.
> 
> @@ +167,5 @@
> >  
> > +/**
> > + * Return a (possibly empty) list of cards
> > + *
> > + * No longer pushes null/empty element into the returned list.
> 
> I don't think we should document what this function _no longer_ does, but
> what it _currently_ does.
> 
> @@ +190,2 @@
> >  
> > +  var cards = [];
> 
> Use let, not var - that applies to all new variables.
> 
> @@ +202,5 @@
> > +      var tmp = abView.getCardFromRow(j);
> > +      if (tmp) {
> > +	cards.push(tmp);
> > +      } else {
> > +        // very strange error!
> 
> I don't think this comment adds much that the error string passed to
> reportError doesn't already provide.

Sorry, I missed the mail reminder. I willchange the patch accordingly.

TIA

Comment 27

[ISHIKAWA, Chiaki]

Attachment: Proposd patch (take 10)

Hi, 

I am uploading a new patch.

(In reply to Mike Conley (:mconley) from comment #25)
> Comment on attachment 8362409 [details] [diff] [review]
> Proposed patch (take 9)
> 
> Review of attachment 8362409 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this looks pretty good! Just some nits to clean up. Also, are you
> ensuring that the addressbook tests are still passing?
> 
> ::: mail/components/addrbook/content/addressbook.js
> @@ +781,5 @@
> >          let selected = GetSelectedAbCards();
> >  
> >          if (selected.length != 1)
> >            return false;
> > + 
> 
> Nit - trailing ws.

Fixed.

> 
> ::: mailnews/addrbook/content/abResultsPane.js
> @@ +130,5 @@
> >      else
> >        cardCnt++;
> >    }
> > +  // Sanity check.
> > +  if (cardCnt == 0 && mailingListCnt == 0)
> 
> The only way this could possibly be true is if count was 0 - but if count
> was 0, we'd never have gotten here. I think we can go ahead and remove this
> check.

Right. The extra check is removed.
 
> @@ +167,5 @@
> >  
> > +/**
> > + * Return a (possibly empty) list of cards
> > + *
> > + * No longer pushes null/empty element into the returned list.
> 
> I don't think we should document what this function _no longer_ does, but
> what it _currently_ does.

I changed the comment somewhat.
 
> @@ +190,2 @@
> >  
> > +  var cards = [];
> 
> Use let, not var - that applies to all new variables.

Now, this is tricky.
But I changed all the usage of |var| except for the global variables into
|let| in this particular file.

I ran |make mozmill| and |make xpcshell-tests| and they didn't produce any new errors, and I think it is safe.
 
> @@ +202,5 @@
> > +      var tmp = abView.getCardFromRow(j);
> > +      if (tmp) {
> > +	cards.push(tmp);
> > +      } else {
> > +        // very strange error!
> 
> I don't think this comment adds much that the error string passed to
> reportError doesn't already provide.

I removed the comment.

TIA

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8399790 [details] [diff] [review]
Proposd patch (take 10)

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

Sorry for the long, long wait here.

I think this is mostly landable. I know I may have harped on you to change vars to lets, but I only meant for code that you were touching directly. Maybe revert some of those changes if you were _only_ changing var to let.

I'm going to push this to try and see what we get.

::: mailnews/addrbook/content/abResultsPane.js
@@ +46,5 @@
>  
>    // If we do have a URI, we want to allow updating the review even if the
>    // URI is the same, as the search results may be different.
>  
> +  let sortColumn = kDefaultSortColumn;

So I notice you're changing a bunch of vars to lets. Unless you're touching the code, I don't care about changing old vars to lets, so these can be reverted.

@@ +61,5 @@
>    }
>    else {
>      if (gAbResultsTree.hasAttribute("sortCol"))
>        sortColumn = gAbResultsTree.getAttribute("sortCol");
> +    let sortColumnNode = document.getElementById(sortColumn);

Same as above until line 74.

@@ +188,5 @@
> +  let cards = [];
> +  let count = abView.selection.getRangeCount();
> +  let current = 0;
> +  for (let i = 0; i < count; ++i) {
> +    let start = new Object;

Let's use:

let start = {};
let end = {};

Nobody really uses new Object anymore.

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Pushed to try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=f047e931ac96

Comment 30

[ISHIKAWA, Chiaki]

(In reply to Mike Conley (:mconley) from comment #28)
> Comment on attachment 8399790 [details] [diff] [review]
> Proposd patch (take 10)
> 
> Review of attachment 8399790 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the long, long wait here.
> 
> I think this is mostly landable. I know I may have harped on you to change
> vars to lets, but I only meant for code that you were touching directly.
> Maybe revert some of those changes if you were _only_ changing var to let.
> 
> I'm going to push this to try and see what we get.
> 

Thank you for your feedback.

I have been running this patch locally when I test full debug version of TB with |make mozmill| and |make xpcshell-tests|. So it should be OK.
Even the unrequested changes of "var" to "let" in other places do not generate bugs. (Yes, I took care to change only the locally used entity to be declared by "let").

Anyway, the errors in the above URL in comment 29 seem to be
related to encoding (probably due to the latest character code handling change
propagated to C-C from M-C.)

TIA

Comment 31

[Wayne Mery (:wsmwk)]

Comment on attachment 8399790 [details] [diff] [review]
Proposd patch (take 10)

suspect mconley is looking for another iteration of your patch

Comment 32

[ISHIKAWA, Chiaki]

(In reply to Wayne Mery (:wsmwk) from comment #31)
> Comment on attachment 8399790 [details] [diff] [review]
> Proposd patch (take 10)
> 
> suspect mconley is looking for another iteration of your patch

Thank you for the reminder.
Since early August, I had a hardware problem which I fixed almost now, but
still have some booting sequence problem after I thought I recovered the disk images more or less.
Once I sort this out completely (not sure when), I would have more time to work on TB features/bug fixes.

TIA

Comment 33

[ISHIKAWA, Chiaki]

Attachment: Proposed patch (take 11).

Finally, I am uploading a new patch now that my hardware issues are almost solved. (I am trying to move to a new VM image, but that is not finished yet.)

Here are some comments inline:
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #28)
> Comment on attachment 8399790 [details] [diff] [review]
> Proposd patch (take 10)
> 
> Review of attachment 8399790 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the long, long wait here.
> 
> I think this is mostly landable. I know I may have harped on you to change
> vars to lets, but I only meant for code that you were touching directly.
> Maybe revert some of those changes if you were _only_ changing var to let.
> 

I still feel a little bit hazy about "let <-> var" thing.

I reverted modified let to var in most places.
But I left the change for a particular pattern
for (var loopvariable = ...) => for (let loopvariable = ...)
in place.
Now that I looked at it, these loopvariable should be only visible
within loopvariable (unless its value is checked AFTER the loop ends.)

> I'm going to push this to try and see what we get.
> 
> ::: mailnews/addrbook/content/abResultsPane.js
> @@ +46,5 @@
> >  
> >    // If we do have a URI, we want to allow updating the review even if the
> >    // URI is the same, as the search results may be different.
> >  
> > +  let sortColumn = kDefaultSortColumn;
> 
> So I notice you're changing a bunch of vars to lets. Unless you're touching
> the code, I don't care about changing old vars to lets, so these can be
> reverted.

As noted above.

> 
> @@ +61,5 @@
> >    }
> >    else {
> >      if (gAbResultsTree.hasAttribute("sortCol"))
> >        sortColumn = gAbResultsTree.getAttribute("sortCol");
> > +    let sortColumnNode = document.getElementById(sortColumn);
> 
> Same as above until line 74.


As noted above.

> 
> @@ +188,5 @@
> > +  let cards = [];
> > +  let count = abView.selection.getRangeCount();
> > +  let current = 0;
> > +  for (let i = 0; i < count; ++i) {
> > +    let start = new Object;
> 
> Let's use:
> 
> let start = {};
> let end = {};
> 
> Nobody really uses new Object anymore.

Did use "{}" here.

I ran the patch through |make mozmill| test locally.
No problems found.

However, I hate to say that in the three months (since June) till I refreshed
my C-C tree a few days ago, so many regressions (in terms of warnings, undefined variable, etc.) crept in,
and JS interpreter/VM seems to check on some things much more strictly
that my run of |make mozmill| of DEBUG BUILD produces so many noisy lines.
Although I did not see any strange errors coming from
this change of mine which I posted here, 
I doubt if you can get a clean, I mean easy to check, log from tryserver as of now...

Several patches that have been checked in the last couple of days and
will be in the next few days would clean up the log, but
still I see valid regressions even after that.

TIA

Comment 34

[ISHIKAWA, Chiaki]

I forgot to mention.
While I was rewriting part of the code, I was reminded that
there ought to be a space between "if" and opening parenthesis as in "if (".
But there are several places in the existing code this was not so.
I changed them, too. Why not clean up some bad coding style violation while your own new code is checked for the same coding style rule?

TIA

Comment 35

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8497630 [details] [diff] [review]
Proposed patch (take 11).

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

I think we should try to take this patch. Our address book selection stuff is really convoluted, and I think this adds some safeties and useful error messages, which is nice. It also (if we remove the error message I point out) will silence the error message on selecting empty mailing lists and address books, which is what this bug is all about.

Thanks for the patch!

::: mailnews/addrbook/content/abResultsPane.js
@@ +187,2 @@
>  
> +  var cards = [];

let instead of var please

@@ +199,5 @@
> +      let tmp = abView.getCardFromRow(j);
> +      if (tmp) {
> +	cards.push(tmp);
> +      } else {
> +	Components.utils.reportError("Error: getCardFromRow(j) returned null: count, start.value, end.value, i, j = " +

So now instead of the card[i] error showing up, we get this one instead. :/

I'm tempted to just silently ignore if we can't get a card from a row, and just push cards into the array iff they exist.

Let's get rid of this reportError. Sorry if I previously asked you to add it.

Comment 36

[ISHIKAWA, Chiaki]

Attachment: Final patch. Carrying over r=mconley+

Here is the final patch.
TIA

Comment 37

[ISHIKAWA, Chiaki]

I put the checkin-needed keyword, but I think it will be some time before the patch
to C-C tree is allowed.

This safety mconley mentioned is what I would like to see in many parts of the code. We can not assume some implicit (or even stated) assumptions forever and sometimes the program breaks unexpectedly because some APIs are changed silently (since it is assumed that it is not relied upon, or some implicit assumptions are not recognized as such).
We ought to be defensive against such changes, thus defensive programming (An old book called Software Tools explained this in layman's terms, but I think it is out of print.) . I think such defensive programming in the form of warning helped during this C-C bustage. I tried to remove many local patches to make sure
the the bustage is not due to my local changes. But at one point, I noticed a mesage printed by my local changes that pointed something funny is going on. Eventually, this led me to investigate TBPL to conclude that the bustage is NOT due to my own changes, but C-C/M-C changes.

TIA

Comment 38

[ISHIKAWA, Chiaki]

Comment on attachment 8507515 [details] [diff] [review]
Final patch.  Carrying over r=mconley+

Oops, I forgot to put the review flag, and edited that it is clear that I am carrying over the "+" flag.

Comment 39

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/11f604e98500 -> FIXED

Please in the future remember to have the whole main message and review comment on the first line of the patch

Comment 40

[ISHIKAWA, Chiaki]

(In reply to Magnus Melin from comment #39)
> https://hg.mozilla.org/comm-central/rev/11f604e98500 -> FIXED
> 
> Please in the future remember to have the whole main message and review
> comment on the first line of the patch

I see. Sorry, I realize I broke the line into a few lines separated by LF.