492216 - On layout change, listener for onEndMsgDownload seems gone, so that never runs.

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[alta88]

consequently, neither does onMsgParsed, and a number of things don't happen anymore that would as a matter of course after selecting a message without a layout change.  most notably, feed messages are no longer visible.

seems nsMsgStatusFeedback isn't restored.  also, onEndMsgDownload normally runs each time a pop or feed message is selected, but not for imap or nntp.  it may or may not be an oversight that it doesn't for imap.

Comment 1

[Magnus Melin [:mkmelin]]

so regression?

Comment 2

[Wayne Mery (:wsmwk)]

alta88, can you estimate or find the regression range??

is bug 535980 a duplicate, or anything else mention there?

Comment 3

[alta88]

i don't know if the reply based bugs are related.  bug 531397#c6 seems to have done some analysis in addition to what i found here (a long time ago).  in that bug i mention a checkin that broke layouts; that would be 781f622cb9b7 of 2009-06-25.  so i'd guess the issue starts there.  despite trying quite hard to figure out how to fix layouts, since widethread is very important to me, i abandoned it with a fried brain..

Comment 4

[Wayne Mery (:wsmwk)]

In reply to comment #3)
> i don't know if the reply based bugs are related.  bug 531397 comment 6 (standard8 comment) seems to have
> done some analysis in addition to what i found here (a long time ago). 

I put regression range at between 20090501 and 20090502.
fixing this should help bug 531397. 
bug 542213 and others should also be checked.(

Comment 5

[Wayne Mery (:wsmwk)]

http://hg.mozilla.org/comm-central/pushloghtml?startdate=2009-05-01+03%3A00&enddate=2009-05-02+05%3A00

I would guess the checkin of Bug 438429 - Meta bug to fix several RSS Summary/Web Page

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Interesting.  If I change layout while on the second (3pane) tab, the second tab's message reader still works, but the first (3pane) tab breaks.  This suggests something bad is happening in terms of the msgWindow associated with the first tab...

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

And I get a this:
WARNING: NS_ENSURE_TRUE(!mIsBeingDestroyed) failed: file /home/visbrero/rev_control/hg/comm-central/mozilla/docshell/base/nsDocShell.cpp, line 7948

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

The nsMessenger's mDocShell is apparently broken.  The messenger instance gets told that by a call to setWindow.  Each mail tab has their own messenger instance, and each one of those needs to issue a new call to setWindow.

not it!

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Jim, you love tabs and folderDisplay, right?  Mayhaps you like this bug?

Comment 10

[Wayne Mery (:wsmwk)]

(In reply to comment #6)
> Interesting.  If I change layout while on the second (3pane) tab, the second
> tab's message reader still works, but the first (3pane) tab breaks.  This
> suggests something bad is happening in terms of the msgWindow associated with
> the first tab...

I hadn't tested in a second tab. But your findings are consistent with the fact that closing 3pane and restarting it "fixes" the problem (and also consistent with reports in other bugs about restarting 3pane, for other types of issues like breakage after replies)

Andrew, Thanks for testing this.

Comment 11

[alta88]

there is bug 636874 for tabs/layout.

Comment 12

[windywater]

I also encountered this bug on version 17.0.8, and fixed it myself.
Andrew Sutherland is right, while changing layout, it goes to function UpdateMailPaneConfig in msgMail3PaneWindow.js, notice this line:
  desiredParent.appendChild(messagePaneBoxWrapper);
The browser is picked up and inserted into a new element, which makes the inside DocShell changed. As the nsMsgStatusFeedback is not registered into the new DocShell, when the message finishes loading, OnEndMsgDownload is no longer reached.
How I fixed:
1. nsMsgWindow::GetMessageWindowDocShell in nsMsgWindow.cpp, change "if (docShell)" into "if (true)", which means always get the messagepane's doc shell, insteading return the old one.
2. function UpdateMailPaneConfig in msgMail3PaneWindow.js, add:
   msgWindow.statusFeedback = msgWindow.statusFeedback;
after 
   desiredParent.appendChild(messagePaneBoxWrapper);
which register the nsMsgStatusFeedback into the new doc shell.

Comment 13

[Magnus Melin [:mkmelin]]

windywater: can you attach a patch for that?

Comment 14

[Geoff Lankow (:darktrojan)]

Attachment: Bug 492216 - Prevent 3-pane layout change from breaking messageHeaderSink listeners

Based on a suggestion at bug 492216 comment 12.

Comment 15

[Geoff Lankow (:darktrojan)]

Well, it does work. Whether or not it's the right thing to do I'm not really sure, but the tests pass. Who's best for reviewing this?

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e75c0826bbee
Prevent 3-pane layout change from breaking messageHeaderSink listeners. r=mkmelin DONTBUILD

Comment 17

[Magnus Melin [:mkmelin]]

Uplift to 78?

Comment 18

[Geoff Lankow (:darktrojan)]

Comment on attachment 9167824 [details]
Bug 492216 - Prevent 3-pane layout change from breaking messageHeaderSink listeners

Ancient bug, no rush, but it's been on beta long enough for problems to surface.

Comment 19

[Wayne Mery (:wsmwk)]

Comment on attachment 9167824 [details]
Bug 492216 - Prevent 3-pane layout change from breaking messageHeaderSink listeners

[Triage Comment]
Approved for esr78

risks?

Comment 20

[Rob Lemley [:rjl]]

Attachment: bug492216_esr78.patch

ESR78 version of this patch necessary due to NS_STRING_LITERAL changes.

Comment 21

[Rob Lemley [:rjl]]

Thunderbird 78.2.2:
https://hg.mozilla.org/releases/comm-esr78/rev/9b53ae7ebe71