Closed Bug 573388 Opened 11 years ago Closed 11 years ago

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

Categories

(MailNews Core :: Backend, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: neil, Assigned: neil)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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.
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?
No, it started recently (say about a week ago).
Would I be correct to assume that this is trunk-only?
Yes, this is both c-c trunk and m-c trunk.
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.
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.
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.
Assignee: nobody → bienvenu
Status: NEW → ASSIGNED
Attachment #486805 - Flags: review?(neil)
Attachment #486805 - Attachment is patch: true
Attachment #486805 - Attachment mime type: application/octet-stream → text/plain
Attached patch Alternative approach (obsolete) — Splinter Review
Given your diagnosis, this is an alternative approach to the problem.

What do you think?
Attachment #486906 - Flags: review?(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 on attachment 486906 [details] [diff] [review]
Alternative approach

r=me, modulo prev comments...
Attachment #486906 - Flags: review?(bienvenu) → review+
Attachment #486805 - Attachment is obsolete: true
Attachment #486805 - Flags: review?(neil)
Seeking sr because of the API addition to nsIMsgWindow.
Assignee: bienvenu → neil
Attachment #486906 - Attachment is obsolete: true
Attachment #487475 - Flags: superreview?(bugzilla)
Attachment #487475 - Flags: review+
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.
Attachment #487475 - Flags: superreview?(bugzilla) → superreview+
Pushed changeset e9aa02dcfc2d to comm-central.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.