Closed Bug 1322838 Opened 4 years ago Closed 4 years ago

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

Categories

(Thunderbird :: Address Book, defect)

defect
Not set
normal

Tracking

(thunderbird52 fixed, thunderbird53 fixed)

RESOLVED FIXED
Thunderbird 53.0
Tracking Status
thunderbird52 --- fixed
thunderbird53 --- fixed

People

(Reporter: aceman, Assigned: jorgk-bmo)

References

Details

Attachments

(1 file, 2 obsolete files)

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;
Attached patch 1322838-sel-dir-check.patch (v1) (obsolete) — Splinter Review
Untested ;-)

Let's not waste too many cycles with this obvious problem.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8817861 - Flags: review?(acelists)
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)
And I also said I will see what the test does.
Sure, but that doesn't affect the correctness of the patch ;-)
That is not so sure. Unless we want to make a new bug if anything needs to be done in addition.
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+
(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.
You can stick r+ onto it again.
Attachment #8817922 - Attachment is obsolete: true
Attachment #8818695 - Flags: review?(acelists)
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+
I will.
https://hg.mozilla.org/comm-central/rev/d91cb69a8e99e16a10a54167652fb9d95527f676
Landed with the comment in the suite/ part fixed.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
Attachment #8818695 - Flags: approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.