498514 - Deleting a message while it's open in another tab causes us to load multiple URLs through the docshell simultaneously

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Rain [:rain1] (inactive)]

STR:

1. Open up several copies of the same message in tabs.
2. Delete the message.
3. Watch how all the URLs we try to load pile up on each other. For best results, use a debug build and enjoy the "Overwriting an existing document channel" assertions (even more fun when you're on Windows and have to click through a dialog box for each assertion by default).

Two possible ways to solve this issue:
1. Play around with dbView.suppressMsgDisplay to disable message display for inactive tabs.
2. Move to a non-multiplexed implementation for the message pane.

Comment 1

[David :Bienvenu]

I'm seeing the wrong message get displayed in the thread pane of a 3-pane tab if I have a single message open in a separate tab. 

STR:
0. (I have my inbox threaded, with newer threads at the top, if it matters)
1. Open a message in the inbox in a separate tab.
2. Shutdown and restart.
3. Select a message in the inbox.
3. Send yourself a message that will end up at the top of the inbox.
4. Hit next unread to read the new message.
5. Press delete

At this point, you'll see the wrong message loaded in the message window. The correct message is selected in the thread pane. In this case, I see the msg that's open in the single message tab loaded in the 3-pane message pane, though I'm pretty sure I've seen other somewhat random messages loaded.

I am seeing two message stream urls running at the same time, which Sid says is related.

I'm marking this a b3 blocker because leaving a message open in a tab seems like a common thing to do, and having the wrong message loaded in the 3-pane is scary at best and could lead to data loss.

Comment 2

[Rain [:rain1] (inactive)]

Attachment: patch

Looks like we had almost everything in place already! We have a bit here <http://mxr.mozilla.org/comm-central/source/mail/base/content/messageDisplay.js#188> that should have caused the db view to not load the URL, but we forgot to return the summarizeSelection result for message tabs.

bienvenu, does this fix the issue?

Comment 3

[David :Bienvenu]

Comment on attachment 386615 [details] [diff] [review]
patch

this doesn't quite fix it for me - I still see the message loaded in the background tab change when I delete messages from the 3-pane ui tab. The first message doesn't seem to cause it, but subsequent ones do. I'll try to look into it, but I've got some other things ahead of it in the queue.

Comment 4

[Rain [:rain1] (inactive)]

Attachment: revised patch with tests, v1

chasing regressions down the rabbit hole isn't any fun at all... ouch.

Looks like sending the update command only when the message is selected actually works out fine, at least for the test cases.

Comment 5

[David :Bienvenu]

I'm running with this now - so far, so good.

Comment 6

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

Comment on attachment 387323 [details] [diff] [review]
revised patch with tests, v1

Rich review: http://reviews.visophyte.org/r/387323/

Exported review:

Looks good!


on file: mail/base/content/folderDisplay.js line 1804
>       // If treeSelection is a real tree selection, it would fire the onselect
>       // event on the box object. Now if we're not active, the box object is
>       // going to be a black hole, and we're never going to get the onselect
>       // event.

I find this comment confusing.  Why is the box object going to be a black
hole?  Is it a good thing that we never get the event?


on file: mail/base/content/messageDisplay.js line 356
>       // We used to use preDisplayedMessage instead of preDisplayedViewIndex,

Replace preDisplayedMessage with "the value of this.displayedMessage prior to
the selectionChanged() call" since the previous temporary has no immediately
obvious meaning in this revision.

Also, a very useful comment!  Thank you for adding it; it greatly simplifies
the review :)


on file: mail/base/content/messageWindow.js line 237
>     dump("in messageWindow clearDisplay, about = " + this.aboutToLoadMessage);

Please lose the debug dump or promote to a better form of logging.  Also, you
forgot the newline! ;)


on file: mail/base/content/messageWindow.js line 240
>     // XXX we should figure out why we're calling window.close() both from here
>     // and from onSelectedMessagesChanged.
>     if (!this.aboutToLoadMessage)
>       window.close();

I think this can go away now... The unit tests pass with it removed.


on file: mail/base/content/messageWindow.js line 279
>     // XXX we should figure out why we're calling window.close() both from here
>     // and from clearDisplay.

And then this can go away...


on file: mail/test/mozmill/folder-display/test-deletion-with-multiple-displays.js line 74
>   // since we don't test window close here, it doesn't really matter how many
>   // messages these have
>   make_new_sets_in_folder(lastMessageFolder, [{count: 4}]);
>   make_new_sets_in_folder(oneBeforeFolder, [{count: 10}]);
>   make_new_sets_in_folder(oneAfterFolder, [{count: 10}]);
>   make_new_sets_in_folder(multipleDeletionFolder1, [{count: 30}]);

No need to change this, but until we get better about JIT-ing everything, the
overhead for message creation does add up, so skimping on messages you don't
need isn't a bad policy.


on file: mail/test/mozmill/folder-display/test-deletion-with-multiple-displays.js line 139
> function _verify_message_is_displayed_in(aFlags, aMessage, aIndex) {

I really appreciate that you factor out the commonality of the tests and do so
in a clean manner!


on file: mailnews/base/src/nsMsgDBView.cpp line 5576
>     // Check if this message is currently selected. If it is, tell the frontend
>     // to be prepared for a delete.

Good call on adding this to the view class.  I tend to be too wary of touching
the view classes.

Comment 7

[Rain [:rain1] (inactive)]

(In reply to comment #6)
> 
> on file: mail/base/content/folderDisplay.js line 1804
> >       // If treeSelection is a real tree selection, it would fire the onselect
> >       // event on the box object. Now if we're not active, the box object is
> >       // going to be a black hole, and we're never going to get the onselect
> >       // event.
> 
> I find this comment confusing.  Why is the box object going to be a black
> hole?  Is it a good thing that we never get the event?

Oops, that comment shouldn't be there at all -- it's a leftover from when I was trying another approach. Thanks for the catch!

Comment 8

[David :Bienvenu]

Comment on attachment 387323 [details] [diff] [review]
revised patch with tests, v1

I know you've made this change locally, but just so it's written down:

you don't want to bail on error here because you won't remove the msg from the view.

+        nsresult rv = mCommandUpdater->UpdateNextMessageAfterDelete();
+        NS_ENSURE_SUCCESS(rv, rv);

Comment 9

[Rain [:rain1] (inactive)]

Pushed: http://hg.mozilla.org/comm-central/rev/5bb1a26308ab

Comment 10

[Rain [:rain1] (inactive)]

Pushed a followup, fixing a typo and a few tests that were checking for the wrong thing. http://hg.mozilla.org/comm-central/rev/9c9440b5a999