1774284 - Uncaught TypeError: can't access property "wrappedJSObject", transaction is null

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Lasana Murray]

STR

1. Create two calendars, Test 1 and Test 2.
2. Add 3 events to Test 1.
3. Select all three then delete them.
4. Then delete calendar Test 2.

Expected Results:
No error thrown.

Actual:

Error is thrown:

_checkWritable resource:///modules/CalTransactionManager.jsm:58
    canUndo resource:///modules/CalTransactionManager.jsm:136
    canUndo chrome://calendar/content/calendar-item-editing.js:715
    isCommandEnabled chrome://calendar/content/calendar-command-controller.js:718
    isCommandEnabled chrome://calendar/content/calendar-tabs.js:105
    isCommandEnabled chrome://messenger/content/tabmail.js:548
    goUpdateCommand chrome://global/content/globalOverlay.js:111
    updateUndoRedoMenu chrome://calendar/content/calendar-item-editing.js:729
    endBatchTransaction chrome://calendar/content/calendar-item-editing.js:707
    deleteOccurrences chrome://calendar/content/calendar-views-utils.js:202
    deleteSelectedEvents chrome://calendar/content/calendar-views-utils.js:515
    doCommand chrome://calendar/content/calendar-command-controller.js:327
    doCommand chrome://calendar/content/calendar-command-controller.js:786
    doCommand chrome://calendar/content/calendar-tabs.js:106
    doCommand chrome://messenger/content/tabmail.js:577
    goDoCommand chrome://global/content/globalOverlay.js:128
    oncommand chrome://messenger/content/messenger.xhtml:1

Comment 1

[Lasana Murray]

Attachment: Bug 1774284 - Remove nsITransactionManager usage from CalTransactionManager. r=#thunderbird-reviewers

Comment 2

[Lasana Murray]

_checkWritable() assumes there won't be a situation where there is no transaction and no batch items stored. It turns out however that doing batch transactions in succession clears the list in beginBatch() resulting in just that.

https://searchfox.org/comm-central/rev/c03173af6dc695ff468c2bf06274cfde35e8cd65/calendar/base/src/CalTransactionManager.jsm#56
https://searchfox.org/comm-central/rev/c03173af6dc695ff468c2bf06274cfde35e8cd65/calendar/base/src/CalTransactionManager.jsm#110

I uncovered some other peculiarities with this like not being able to undo anymore if you deleted a calendar (we should skip that operation and let the others be available) as well as some errors thrown when you do things a certain way. Storing the transactions that were in a batch was meant to get around nsITransactionManager.peekUndoStack() returning null when a batch transaction was created.

Looking at the C++ code, I don't think nsITransactionManager provides any major benefits or features and we already re-implement some of the functionality so I removed its use for a pure JS implementation. This way we can better support the async nature of these operations.

Comment 3

[Lasana Murray]

Attachment: Bug 1774284 - Add unit tests for the CalTransactionManager and related classes. r=#thunderbird-reviewers

Depends on D149579

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9b9d520bce2e
Remove nsITransactionManager usage from CalTransactionManager. r=thunderbird-reviewers,freaktechnik
https://hg.mozilla.org/comm-central/rev/0c0558af4b45
Add unit tests for the CalTransactionManager and related classes. r=thunderbird-reviewers,freaktechnik

Comment 5

[Magnus Melin [:mkmelin]]

I'd assume we should uplift?

Comment 6

[Magnus Melin [:mkmelin]]

(In reply to Magnus Melin [:mkmelin] from comment #5)

I'd assume we should uplift?

Comment 7

[Lasana Murray]

Sure. I have not observed any problems with it since.

Comment 8

[Lasana Murray]

Comment on attachment 9281721 [details]
Bug 1774284 - Remove nsITransactionManager usage from CalTransactionManager. r=#thunderbird-reviewers

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: Occasional JS errors when executing calendar operations.
Testing completed (on c-c, etc.): comm-central
Risk to taking this patch (and alternatives if risky): Low risk since it has been behaving. There is potential for undo/redo to not work if anything is wrong.

Comment 9

[Lasana Murray]

Comment on attachment 9281759 [details]
Bug 1774284 - Add unit tests for the CalTransactionManager and related classes. r=#thunderbird-reviewers

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: None
Testing completed (on c-c, etc.): comm-central
Risk to taking this patch (and alternatives if risky): Low risk, just a test.

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9281721 [details]
Bug 1774284 - Remove nsITransactionManager usage from CalTransactionManager. r=#thunderbird-reviewers

(This has been on beta since 103)

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9281759 [details]
Bug 1774284 - Add unit tests for the CalTransactionManager and related classes. r=#thunderbird-reviewers

(This has been on beta since 103)

Comment 12

[Wayne Mery (:wsmwk)]

Comment on attachment 9281721 [details]
Bug 1774284 - Remove nsITransactionManager usage from CalTransactionManager. r=#thunderbird-reviewers

[Triage Comment]
Approved for esr102

Comment 13

[Wayne Mery (:wsmwk)]

Comment on attachment 9281759 [details]
Bug 1774284 - Add unit tests for the CalTransactionManager and related classes. r=#thunderbird-reviewers

[Triage Comment]
Approved for esr102

Comment 14

[Rob Lemley [:rjl]]

Thunderbird 102.2.1:
https://hg.mozilla.org/releases/comm-esr102/rev/41c4d5ea5af5
https://hg.mozilla.org/releases/comm-esr102/rev/7d14277eb478