1322838 - selectedDir is null in chrome://messenger/content/addressbook/abCommon.js

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[:aceman]

Can be seen in all test runs:
20:01:03     INFO -  TEST-START | C:\slave\test\build\tests\mozmill\addrbook\test-update-mailing-list.js | test_contact_in_mailing_list_updated
20:01:03     INFO -  JavaScript error: chrome://messenger/content/addressbook/abCommon.js, line 69: TypeError: selectedDir is null
20:01:03     INFO -  JavaScript error: chrome://messenger/content/addressbook/abCommon.js, line 69: TypeError: selectedDir is null
20:01:03     INFO -  JavaScript error: chrome://messenger/content/addressbook/abCommon.js, line 69: TypeError: selectedDir is null
20:01:03     INFO -  JavaScript error: chrome://messenger/content/addressbook/abCommon.js, line 69: TypeError: selectedDir is null

Caused in bug 1319409.

selectedDir.URI is called before selectedDir is checked for null, that is done later in the code.

Please check the Seamonkey version too.

Comment 1

[Thomas D. (:thomas8)]

Thanks. The code below is obviously wrong, because if we're testing for selectedDir == null, we must do that before getting selectedDir.URI. Which is nice because it simplifies the code even more. But I suspect the test code is also wrong: There's no way in the directory tree UI to NOT have a selected directory. Un-selecting a directory is not possible. So how does test code arrive at a scenario without a selected directory?
Can you file a link to the test code?


      case "button_delete": {
        let selectedDir = getSelectedDirectory();
        let selectedDirURI = selectedDir.URI;

        // Context-sensitive labels for Edit > Delete menuitem.
        if (command == "cmd_delete" && selectedDir) {
          goSetMenuValue(command, selectedDir.isMailList ?
                                  "valueList" : "valueAddressBook");
        }

        // If there's no directory selected, or if it's one of these special ABs,
        // return false to disable deletion.
        if (!selectedDir ||
             (selectedDirURI == kPersonalAddressbookURI) ||
             (selectedDirURI == kCollectedAddressbookURI))
          return false;

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1322838-sel-dir-check.patch (v1)

Untested ;-)

Let's not waste too many cycles with this obvious problem.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1322838-sel-dir-check.patch (v1b).

Aceman told me on IRC that I need to |return false;|, so here we go.

Comment 4

[:aceman]

And I also said I will see what the test does.

Comment 5

[Jorg K (CEST = GMT+2)]

Sure, but that doesn't affect the correctness of the patch ;-)

Comment 6

[:aceman]

That is not so sure. Unless we want to make a new bug if anything needs to be done in addition.

Comment 7

[:aceman]

Comment on attachment 8817922 [details] [diff] [review]
1322838-sel-dir-check.patch (v1b).

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

OK, the error is not easily reproducible on Linux. But I've looked at the test and it does delete the currently selected AB which may trigger the error. I could also reproduce it manually. Also creating new AB in the AB window triggers the error. It may be that for a moment when the AB tree is rebuilt or modified, that there is temporarily no selection.

The patch fixes these cases so I do not see anything more that would need to be done here.

Comment 8

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #7)
> The patch fixes these cases so I do not see anything more that would need to
> be done here.
Sigh, I do. Fix the suite/ part. We landed the same stuff that doesn't work on suite/

I'll do the same change in suite/. Do you want to see it again?

So much for Thomas' "risk free" patch. I'm glad I didn't uplift that.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1322838-sel-dir-check.patch (v1b) + suite/

You can stick r+ onto it again.

Comment 10

[:aceman]

Comment on attachment 8818695 [details] [diff] [review]
1322838-sel-dir-check.patch (v1b) + suite/

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

Thanks, looks fine.

::: suite/mailnews/addrbook/abCommon.js
@@ +64,5 @@
>            goSetMenuValue(command, selectedDir.isMailList ?
>                                    "valueList" : "valueAddressBook");
>          }
>  
>          // If there's no directory selected, or if it's one of these special ABs,

You didn't update the comment in the way you did for TB.

Comment 11

[Jorg K (CEST = GMT+2)]

I will.

Comment 12

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/d91cb69a8e99e16a10a54167652fb9d95527f676
Landed with the comment in the suite/ part fixed.

Comment 13

[Jorg K (CEST = GMT+2)]

Aurora (TB 52):
https://hg.mozilla.org/releases/comm-aurora/rev/06d0c74bf867ad62fc6c50f4e5724544522013bf (regressing bug)
https://hg.mozilla.org/releases/comm-aurora/rev/1e40d7501405429e0f88adefc3d096c59464dc56 (this bug)

This needed uplift since the regressing bug was uplifted, see bug 1319409 comment #60.