The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 53.0

Status

Thunderbird
Address Book
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: aceman, Assigned: Jorg K (GMT+1))

Tracking

Trunk
Thunderbird 53.0

Thunderbird Tracking Flags

(thunderbird52 fixed, thunderbird53 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 months ago
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.
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;
(Assignee)

Comment 2

3 months ago
Created attachment 8817861 [details] [diff] [review]
1322838-sel-dir-check.patch (v1)

Untested ;-)

Let's not waste too many cycles with this obvious problem.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8817861 - Flags: review?(acelists)
(Assignee)

Comment 3

3 months ago
Created attachment 8817922 [details] [diff] [review]
1322838-sel-dir-check.patch (v1b).

Aceman told me on IRC that I need to |return false;|, so here we go.
Attachment #8817861 - Attachment is obsolete: true
Attachment #8817861 - Flags: review?(acelists)
Attachment #8817922 - Flags: review?(acelists)
(Reporter)

Comment 4

3 months ago
And I also said I will see what the test does.
(Assignee)

Comment 5

3 months ago
Sure, but that doesn't affect the correctness of the patch ;-)
(Reporter)

Comment 6

3 months ago
That is not so sure. Unless we want to make a new bug if anything needs to be done in addition.
(Reporter)

Comment 7

3 months ago
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.
Attachment #8817922 - Flags: review?(acelists) → review+
(Assignee)

Comment 8

3 months ago
(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.
(Assignee)

Comment 9

3 months ago
Created attachment 8818695 [details] [diff] [review]
1322838-sel-dir-check.patch (v1b) + suite/

You can stick r+ onto it again.
Attachment #8817922 - Attachment is obsolete: true
Attachment #8818695 - Flags: review?(acelists)
(Reporter)

Comment 10

3 months ago
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.
Attachment #8818695 - Flags: review?(acelists) → review+
(Assignee)

Comment 11

3 months ago
I will.
(Assignee)

Comment 12

3 months ago
https://hg.mozilla.org/comm-central/rev/d91cb69a8e99e16a10a54167652fb9d95527f676
Landed with the comment in the suite/ part fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
(Assignee)

Comment 13

2 months ago
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.
status-thunderbird52: --- → fixed
status-thunderbird53: --- → fixed
(Assignee)

Updated

2 months ago
Attachment #8818695 - Flags: approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.