573388 - ASSERTION: aWebProgress and channel callbacks do not point to the same docshell

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[[e-mail]]

I noticed this assertion while reading a message.

aWebProgress is the content docShell for the IMAP url.

However the channel callbacks are the chrome docShell for the mail window.

The assertion was introduced by bug 374577.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

The code that does the asserting was introduced by bug 374577, and modified a bit by 479906.  Is it your belief that this assertion has been triggered all along, or that this is new?

Comment 2

[[e-mail]]

No, it started recently (say about a week ago).

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Would I be correct to assume that this is trunk-only?

Comment 4

[[e-mail]]

Yes, this is both c-c trunk and m-c trunk.

Comment 5

[David :Bienvenu]

I see this happen if I click on imap messages while autosync is downloading other imap messages in the same folder. I'm wondering if this somehow related to the imap disk cache corruptions and/or offline store corruptions.

Comment 6

[David :Bienvenu]

Actually, I see this whenever we fetch an imap message from the server for display, as opposed to using the disk cache or memory cache or offline store.

Comment 7

[David :Bienvenu]

Attachment: quiet the assertion by using the same docshell

Neil, what do you think of this approach? This makes us use the loading docshell when setting up the notification callbacks, if available, which makes the content policy code less unhappy. I checked that the cert exception override dialog is still displayed when clicking on a message, if there's no cert exception.

Comment 8

[[e-mail]]

Attachment: Alternative approach

Given your diagnosis, this is an alternative approach to the problem.

What do you think?

Comment 9

[David :Bienvenu]

Comment on attachment 486906 [details] [diff] [review]
Alternative approach

yes, I thought about that alternative in the middle of the night. It's definitely nicer. I think you want to return an error if we can't get the docshell, instead of always returning NS_OK, and unless this comment isn't valid anymore, it should be propagated to the nsIMsgWindow.idl:

-  // on the message window docshell. Under no circumstances should you be holding on to
-  // the docshell returned here outside the scope of your routine.


I'll give the patch a try.

Comment 10

[David :Bienvenu]

Comment on attachment 486906 [details] [diff] [review]
Alternative approach

r=me, modulo prev comments...

Comment 11

[[e-mail]]

Attachment: Addressed review comments

Seeking sr because of the API addition to nsIMsgWindow.

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 487475 [details] [diff] [review]
Addressed review comments

So I kinda prefer David's solution on the fact it doesn't rely on the hard-coded "messagepane" element. However, looking at where GetMessageWindowDocShell is used currently, there's a bigger set of issues to resolve to avoid that hard-coding. Therefore I think we'll take this for now and look at those issues later.

Comment 13

[[e-mail]]

Pushed changeset e9aa02dcfc2d to comm-central.