Closed Bug 1518823 Opened 3 years ago Closed 3 years ago

Port Bug 1482389 Convert TreeBoxObject to XULTreeElement - Malfunction: Deletion of message opened in tab or stand-alone windows not working as expected

Categories

(MailNews Core :: General, task)

task
Not set
blocker

Tracking

(thunderbird68+ fixed, thunderbird69 fixed)

RESOLVED FIXED
Thunderbird 66.0
Tracking Status
thunderbird68 + fixed
thunderbird69 --- fixed

People

(Reporter: jorgk-bmo, Assigned: darktrojan)

References

Details

(Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test][XBL:31])

Attachments

(5 files, 11 obsolete files)

12.97 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
110.61 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
2.11 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
26.03 KB, patch
Details | Diff | Splinter Review
37.57 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review

Wow, that code got interpreted as formatting :-(

-      tree.treeBoxObject.ensureRowIsVisible(i);
+      tree.ensureRowIsVisible(i);

This appears to compile so far, I commented out three lines with XXX that resisted compilation. Needs further looking at.

Assignee: nobody → jorgk
Status: NEW → ASSIGNED

This compiles, and no more commented-out code :-)

Over to the JS part. I'm going to do what I said in comment #2, so s/treeBoxObject.ensureRowIsVisible/ensureRowIsVisible/.

Paolo, what about treebox.ensureRowIsVisible()? That's something else? Sorry, pattern-matchers at work ;-)

Attachment #9035353 - Attachment is obsolete: true
Attachment #9035386 - Flags: review?(paolo.mozmail)

Wow, the JS part is going to be massive, all access via .treeBoxObject needs to go.

Magnus, we have a problem here. nsITreeBoxObject is removed and we still have it in FakeTreeBoxObject in folderDisplay.js.

JS patch coming in a minute.

Flags: needinfo?(mkmelin+mozilla)
Attached patch 1518823-treebox.patch - v0 (obsolete) — Splinter Review

This is what I have so far. The folder tree displays, but the thread pane/message list doesn't.

I see:
this.treeBox.getFirstVisibleRow is not a function folderDisplay.js:1742
TypeError: treeBox.getFirstVisibleRow is not a function folderDisplay.js:2473:17

Attached patch 1518823-treebox.patch - v1 (obsolete) — Splinter Review

OK, with this patch, the thread pane has returned.

I've also looked at
https://searchfox.org/comm-central/search?q=treeBox.ensureRowIsVisible&case=false&regexp=false&path=

'treeBox' was an object property here. I've renamed it to 'tree'.

Attachment #9035404 - Attachment is obsolete: true
Attachment #9035410 - Flags: feedback?(mkmelin+mozilla)
Attachment #9035410 - Flags: feedback?(acelists)
Comment on attachment 9035410 [details] [diff] [review]
1518823-treebox.patch - v1

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

Looks reasonable to me
Attachment #9035410 - Flags: feedback?(mkmelin+mozilla)
Attachment #9035410 - Flags: feedback?(acelists)
Attachment #9035410 - Flags: feedback+

Oops, never sent this:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=51ddd5d04999b44262dec60685c7e7da1a506d21

It's totally red. Locally I see:
JavaScript error: chrome://messenger/content/messageWindow.js, line 105: NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 0 [nsITreeView.setTree]

Also, Geoff pointed out that there is treeBoxObject in XML files :-(

Grrr, messageWindow.js, line 105 is that fake stuff that is biting us now :-(

this.view.dbView.setTree(this._fakeTreeBox);

Attached patch 1518823-treebox.patch - v2 (obsolete) — Splinter Review

OK, this elimitates treeBoxObject from mailWidgets.xml. I don't think it will make test better, but we can see:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c8b8d4e5a412f1553776d38aff40814ec2f791f9

For the "fake stuff", take a look at bug 1434641 comment #6 downwards.

Attached patch 1518823-treebox.patch - v3 (obsolete) — Splinter Review
Attachment #9035410 - Attachment is obsolete: true
Attachment #9035445 - Attachment is obsolete: true

I still see
NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 0 [nsITreeView.setTree] folderDisplay.js:1776

Looks like the removal of nsITreeBoxObject also took away the possibility of simulating a fake tree in JS, right?

OK, now with:
1776: // this.view.dbView.setTree(this._fakeTree);

Let's see:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4455b52bda0236de9dcb19b0cb0e4e0a2b54bed2

And another one with folderDisplay.js also
105: // this.view.dbView.setTree(this._fakeTree);

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=dbe15f6142977dad8ec21baa8ec05e736344fb98

(In reply to Jorg K (GMT+1) from comment #15)

I still see
NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 0 [nsITreeView.setTree] folderDisplay.js:1776

Looks like the removal of nsITreeBoxObject also took away the possibility of simulating a fake tree in JS, right?

Does your change fake object QI to the right interface?

No, it doesn't, see comment #6. It's the:
https://searchfox.org/comm-central/rev/58f568fcebd755ece1b6c6622c45a62decd481d7/mail/base/content/folderDisplay.js#2662

But with the removal of nsITreeBoxObject, the XPCOM-ness is gone, no? What should I QI to?

The IDL says XULTreeElement: https://hg.mozilla.org/try/rev/59b0eb21a4fb#l53.31

But how you do that and whether it actually matters or not, I don't know.

I also get:
JavaScript error: chrome://messenger/content/threadPane.js, line 70: TypeError:
col.value is undefined
JavaScript error: chrome://messenger/content/threadPane.js, line 64: TypeError:
col.value is undefined

That's from tree.getCellAt(event.clientX, event.clientY, row, col, elt);

(In reply to Geoff Lankow (:darktrojan) from comment #20)

The IDL says XULTreeElement: https://hg.mozilla.org/try/rev/59b0eb21a4fb#l53.31
But how you do that and whether it actually matters or not, I don't know.

Yes, that let's me remove the
// this.view.dbView.setTree(this._fakeTree);

Let's try again, but the errors from comment 21 don't help :-(
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=43f09c2ebcf540656adf12af734c3cebe141aa82

Attached patch 1518823-treebox.patch - v4 (obsolete) — Splinter Review

OK, fixed the QI and all the getCellAt() calls.

Attachment #9035461 - Attachment is obsolete: true
Attached patch disable-fake.patch (obsolete) — Splinter Review

If I throw this in, the thread pane shows and I can open a message in a tab and a new window. Closing the tab crashes, but hey, you can't have everything.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7c2eaa3b4e7eedaf04422aca0ed7775ede2f9bff

Fixed another typo in a separate patch:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=2c0781e246e6b9d912197eaefb53bf7c34a59f37

Note: Since this is not based on "normal" M-C but on a try changeset, the jobs can't be cancelled :-( - well, or only individually with the TaskCluster tool.

Attached patch 1518823-treebox.patch (v5) (obsolete) — Splinter Review

OK, I've folded the misc. fixes into the main patch here.

With disable-fake.patch the try had three of the four Z's green (modulo the pesky testTaskView.js).

Z2 is red with multiple failures in four tests:
folder-display/test-deletion-from-virtual-folders.js
folder-display/test-deletion-with-multiple-displays.js
folder-display/test-folder-pane-visibility.js
folder-display/test-pane-focus.js
folder-display/test-close-window-on-delete.js

Magnus, what's the way forward here?

Attachment #9035503 - Attachment is obsolete: true

Sigh, another two call sites of getCellAt() needed fixing. Here's another try with the fake stuff disabled:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=717849595eb4a84c7f00169b3bc1e9b4bfdf8ecb

Attachment #9035549 - Attachment is obsolete: true

Looking, but doesn't look like fun.

Looks like the try form comment #29 is worse than the one from comment #27 :-(

Comment on attachment 9035386 [details] [diff] [review]
1518823-nsITreeBoxObject.patch (v1)

(In reply to Jorg K (GMT+1) from comment #4)

Paolo, what about treebox.ensureRowIsVisible()?

What's been done in this bug seems alright. Methods like this one that were on nsITreeBoxObject are now called on the <tree> element. As for FakeTreeBoxObject, probably you have to look into why it exists and whether there's an alternative way of achieving the same effect. In some cases I found that complex code could be converted to a one-liner in the original XBL binding, and I think we can consider a patch for a solution like this for the <tree> element if necessary.

Attachment #9035386 - Flags: review?(paolo.mozmail)

Sadly there are too many bugs involved in this area. In M-C we have bug 1434641 and bug 1482389, in C-C we have this bug here and also bug 1514624 which removed a snippet.

The main reading re. the "fake stuff" is in bug 1434641 comment #6 and further down (cmt 8 and 12).

Comment on attachment 9035386 [details] [diff] [review]
1518823-nsITreeBoxObject.patch (v1)

This will have to land soon.

Attachment #9035386 - Flags: review?(mkmelin+mozilla)
Attachment #9035552 - Flags: review?(mkmelin+mozilla)

This is sadly necessary.

Attachment #9035504 - Attachment is obsolete: true
Attachment #9036528 - Flags: feedback?(mkmelin+mozilla)

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8015b21deff6
No bug - Disable Nightly builds until XUL tree issue is fixed. a=jorgk DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Keywords: leave-open
Resolution: FIXED → ---
Assignee: jorgk → nobody
Status: UNCONFIRMED → NEW
Ever confirmed: true
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/85114397b4c8
Port bug 1482389: Replace nsITreeBoxObject with XULTreeElement. rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/96b12a150fe1
Port bug 1482389: Remove acces via .treeBoxObject and .boxObject. rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/561033e56bac
Port bug 1482389: Disable assigning fake JS tree. rs=bustage-fix

BTW, message display in a tab works and "b", "f" and "n" (what does that do?) appear to work.

Attachment #9035386 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9035552 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9036528 - Flags: feedback?(mkmelin+mozilla) → review+

(In reply to Jorg K (GMT+1) from comment #38)

BTW, message display in a tab works and "b", "f" and "n" (what does that
do?) appear to work.

Go to previous, next, next unread message.

Flags: needinfo?(mkmelin+mozilla)

Andrew, thanks for the explanation in bug 1434641 comment #6 and further down.

Would it be possible to move the implementation of the "fake tree" to C++ code?
https://searchfox.org/comm-central/rev/a8aec0d10f547e8181430a41db331e710a5c9382/mail/base/content/folderDisplay.js#2588

As I understand the immediate problem, nsITreeBoxObject is no longer an XPCOM object, so the tree can't be implemented in JS any more, that's why we had to disable these assignments: https://hg.mozilla.org/comm-central/rev/561033e56bac

For the uninitiated, your comment six in the other bug is hard to digest since it refers to the "pre-tab" times and mentions a completing a "change-over".

Flags: needinfo?(bugmail)
Attached patch 1518823-disable-tests.patch (obsolete) — Splinter Review

Not the standard way to turn off tests, but this was quickest.

Attachment #9036659 - Attachment is obsolete: true
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b18b1440b89e
Backed out changeset 8015b21deff6 to re-enable Nightly builds . a=jorgk
https://hg.mozilla.org/comm-central/rev/a77580b97618
Bulk turn off all subtests in three failing tests. rs=bustage-fix

(In reply to Jorg K (GMT+1) from comment #40)

Would it be possible to move the implementation of the "fake tree" to C++ code?
https://searchfox.org/comm-central/rev/a8aec0d10f547e8181430a41db331e710a5c9382/mail/base/content/folderDisplay.js#2588

There's no reason the fake tree need be implemented in JS. It was just very easy and there were no scaling problems. If you can implement a C++ subclass and aren't going to run into problems where methods aren't virtual, then that does seem like a way to maintain the status quo.

As I understand the immediate problem, nsITreeBoxObject is no longer an XPCOM object, so the tree can't be implemented in JS any more, that's why we had to disable these assignments: https://hg.mozilla.org/comm-central/rev/561033e56bac

I'm not really following the work in the other bugs, just trying to provide context.

For the uninitiated, your comment six in the other bug is hard to digest since it refers to the "pre-tab" times and mentions a completing a "change-over".

Thunderbird used to not have any tabs. Then tabs were implemented as a major hack where the user thought they had N 3-pane UI's working as separate, parallel things. But in fact it's just one set of widgets having hacky things done to them when the tab changes. The Folder Display abstractions were created to normalize the hack and provide a (more) sane API to add-ons as a first step to changing-over to not having any hack.

Flags: needinfo?(bugmail)
Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test]
Duplicate of this bug: 1522460

I'm going to see whether changing the FakeTree implementation in folderDisplay.js to C++ is going to fix the immediate issue and maintain the status quo. That's a lot of boiler plate to be written :-(

I got confused and spent some time in the wrong boat :-(

FakeTree doesn't implement a "tree view" (nsITreeView) which we could extend, but the former nsITreeBoxObject which was replaced by the final class XULTreeElement:
https://searchfox.org/comm-central/rev/3d98ee9af358660c44644d24b738f17261165ca6/mozilla/dom/xul/XULTreeElement.h#29

So nothing can be derived from it and it mostly doesn't have virtual methods.

Duplicate of this bug: 1528500
Target Milestone: --- → Thunderbird 66.0
Duplicate of this bug: 1530216
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/978efb2e64bc
Follow-up: Fix left-over 'col.value' breaking the subscribe tree. r=me DONTBUILD

TB 66 beta 3:
https://hg.mozilla.org/releases/comm=beta/rev/df1d9106a00f4c8436a0256750ae9882a1c6c478

EDIT: Destroyed the correct links do the c-b landing doesn't prevent this from showing up in my uplift query.

Duplicate of this bug: 1531646
Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test] → [Thunderbird-testfailure: Z all][Thunderbird-disabled-test][XBL:40]

Tests disabled here:
test-close-window-on-delete.js - 7
test-deletion-from-virtual-folders.js - 12
test-deletion-with-multiple-displays.js - 21

Geoff notices that 9 can be re-enabled since they pass now or never failed and were just a victim of hasty whole-sale disabling. Thanks Geoff. There will still be tests disabled in all three files.

Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test][XBL:40] → [Thunderbird-testfailure: Z all][Thunderbird-disabled-test][XBL:31]
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1fa6d6d6dd6c
Re-enable 9 tests that now pass (or never failed). r=jorgk DONTBUILD
Duplicate of this bug: 1541151
Duplicate of this bug: 1541159
Duplicate of this bug: 1540156

Magnus, what's the way forward here? Can you put one person from your de-XBL team onto this job. Duplicates are piling up. The malfunction is that deletion of messages opened in a tab or stand-alone window doesn't behave as expected any more.

Flags: needinfo?(mkmelin+mozilla)
Summary: Port Bug 1482389 Convert TreeBoxObject to XULTreeElement → Port Bug 1482389 Convert TreeBoxObject to XULTreeElement - Malfuncion: Deletion of message opened in tab or stand-alone windows not working as expected
Summary: Port Bug 1482389 Convert TreeBoxObject to XULTreeElement - Malfuncion: Deletion of message opened in tab or stand-alone windows not working as expected → Port Bug 1482389 Convert TreeBoxObject to XULTreeElement - Malfunction: Deletion of message opened in tab or stand-alone windows not working as expected
Regressions: 1546604
Duplicate of this bug: 1551973
Severity: normal → blocker
Attached patch 1518823-deletion-1.diff (obsolete) — Splinter Review

This isn't very polished and it may well cause other problems, but it does cause 20 previously-failing tests to pass.

I've done two Try runs with some retriggering and not had a green Z2 on Linux yet. (Green on Mac first time on both runs.) I don't think that is related, but it could be.

Ben I'd be interested to know if attachment 9067268 [details] [diff] [review] would solve your bug 1551973.

Flags: needinfo?(ben.bucksch)
Attached patch 1518823-deletion-2.diff (obsolete) — Splinter Review

All tests fixed. This try run has very similar code, but I'll do another with the final changes just to make sure.

Assignee: nobody → geoff
Attachment #9067268 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(ben.bucksch)
Attachment #9068288 - Flags: review?(mkmelin+mozilla)

Wow, "all tests fixed" is referring to all the 31 tests, not just the 20 from the previous patch. Fantastic!!

Linux debug shows a failure (doesn't it always?…), but I'm pretty sure it's just doing things out-of-order and I can fix it by making the test wait for the expected value to appear.

Comment on attachment 9068288 [details] [diff] [review]
1518823-deletion-2.diff

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

I get an area under the status bar, which is apparently showing the fake tree? 

Besides that, does seem to work properly.

::: mail/base/content/folderDisplay.js
@@ +1204,1 @@
>        return;

seems like an odd change to declare the shouldReturn. Isn't the only change the this._deleteInProgress = false; addition?

::: mail/base/content/messageDisplay.js
@@ +504,5 @@
>      // close the tab.
>      if (this.folderDisplay.view.dbView.rowCount == 0) {
>        if (!this.closing) {
>          this.closing = true;
> +        setTimeout(() => {

why the timeouts?
Attachment #9068288 - Flags: review?(mkmelin+mozilla)
Attachment #9068288 - Flags: review-
Attachment #9068288 - Flags: feedback+

Geoff: Thanks for working on this!

> +        setTimeout(() => {

Yes, please never use timeouts in production code. Esp. never ever as a workaround. They are fine in test code, but not in the real app code.

Regressions: 1555341
No longer regressions: 1555341

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

I get an area under the status bar, which is apparently showing the fake
tree?

It should have a height of 0. Why doesn't it?

seems like an odd change to declare the shouldReturn. Isn't the only change
the this._deleteInProgress = false; addition?

_deleteInProgress needs to be false, after calling messageDisplay.onMessagesRemoved. You have a better way of doing it?

why the timeouts?

It seems like something things are happening out-of-order. At these points I think the selected message in the tree has changed but whatever the title-setting code needs is further down the event loop.

(In reply to Geoff Lankow (:darktrojan) from comment #68)

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

I get an area under the status bar, which is apparently showing the fake
tree?

It should have a height of 0. Why doesn't it?

Can't say. Dev tools says the element is 46px, but is confused as to why. Using display: none hides it though.

When starting up with a few messages open in tabs, I got a few of these trees down there, and then some infinite jumping around.

why the timeouts?

It seems like something things are happening out-of-order. At these points I
think the selected message in the tree has changed but whatever the
title-setting code needs is further down the event loop.

Would be good to track this down, or at the minimum add a TODO item linking to a bug to do that, and some explanation.

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

Can't say. Dev tools says the element is 46px, but is confused as to why. Using display: none hides it though.

Using display: none also stops it from working.

visibility: collapse seems to be okay. The extra tree view never showed for me but it did on TaskCluster, sometimes. Anyway, this works:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=23f4601465a50e193861ef2d910761657e3047d3

Attachment #9068288 - Attachment is obsolete: true
Attachment #9068897 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9068897 [details] [diff] [review]
1518823-deletion-3.diff

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

LGTM, r=mkmelin

::: mail/base/content/folderDisplay.js
@@ +1197,5 @@
>    onMessagesRemoved() {
>      FolderDisplayListenerManager._fireListeners("onMessagesRemoved",
>                                                  [this]);
>  
> +    let shouldReturn = this.messageDisplay.onMessagesRemoved();

maybe name it handled, instead of shouldReturn
Attachment #9068897 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9068897 [details] [diff] [review]
1518823-deletion-3.diff

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

::: mail/base/content/folderDisplay.js
@@ +1199,5 @@
>                                                  [this]);
>  
> +    let shouldReturn = this.messageDisplay.onMessagesRemoved();
> +    this._deleteInProgress = false;
> +    if (shouldReturn)

Umm, can't you move `this._deleteInProgress = false;` up and then:
```
if (this.messageDisplay.onMessagesRemoved())
  return;
```
like it was?

I asked earlier, but looking more, I suspect not since onMessagesRemoved checks that.

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

::: mail/base/content/folderDisplay.js
@@ +1197,5 @@

onMessagesRemoved() {
FolderDisplayListenerManager._fireListeners("onMessagesRemoved",
[this]);

  • let shouldReturn = this.messageDisplay.onMessagesRemoved();

maybe name it handled, instead of shouldReturn

Yeah go on, rename it. I'm not fussy.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8c7373a378b6
Replace FakeTree with a real tree and re-enable 31 disabled tests. r=mkmelin DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Landed with the variable renamed. I'll leave the target milestone at TB 66 since that's when the initial stuff landed. The final patch landed at TB 69 and will be backported to TB 68.

Attachment #9068897 - Flags: approval-comm-beta+

I've built trunk and verified that this indeed fixes bug 1551973. That was the worst regression in TB 68, and highly annoying, so thanks a lot for fixing this!

when will this fix be available to those of us running 67.0b3 32 bit version

It will ship in 68.0 beta 1 next week.

Duplicate of this bug: 1558684

Thank you for fixing this! I quit using Daily because of it. Drove me nuts. Now I will go back.

Duplicate of this bug: 1532621
Duplicate of this bug: 1528364
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.