632390 - Message reading and context menu can get broken when opening content tabs

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Mark Banner (:standard8)]

STR:

1) Restart Thunderbird
2) Select a message thread so that the multi message summary view is displayed
3) Open the add-ons manager or some other content tab
4) Switch back to the 3-pane mail tab

At this point the context menu doesn't update correctly (and you get errors about content being null) and sometimes with the Conversations add-on, message reading can be broken as well.

The issue appears to be that window.content is ending up as null. window.content normally tracks the content of the browser element with type "content-primary".

I've checked that we set "content-targetable" on elements before "content-primary" and that seems fine, but still fails.

It appears only to happen with the multi-message view selected (in step 2) - a chrome browser - not the single message view - the content browser.

So I'm currently confused as to what is happening.

Comment 1

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

Maybe nsContextMenu just shouldn't be accessing "content" and should instead either be explicitly contextualized through ownership and/or walking up the DOM tree (possibly through multiple iframes) until it hits the appropriate semantic content shell?  I presume the current use of "content" is a cheat to avoid the cost of performing the walking?

In general, constantly poking at DOM attributes to affect a de facto global seems more error prone than just poking at pure JS globals since we at least have a guarantee of the semantics of (pure) JS globals.  Also, accessing globals set as we switch tabs is still not a great idea either...

Comment 2

[Mark Banner (:standard8)]

Attachment: The fix

I was going to complain about window.content being needed and things but then I realised it probably wouldn't make sense with chrome browsers and things anyway.

In any case, I took a look at the browser/ version of nsContextMenu.js and a look at ours:

- The HTTPIndex code could be removed as per bug 453793.
- inFrame wasn't actually used anywhere, if it is needed later we can just copy from FF and get something which doesn't use content.
- The openInBrowser doesn't have an equivalent, but I based it on some code that Firefox had elsewhere. This should mean it will work where required.

Comment 3

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

Comment on attachment 512598 [details] [diff] [review]
The fix

Thank you for your foray into this generally unpleasant area!

Comment 4

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/67e8ac2dfe85

Comment 6

[Mark Banner (:standard8)]

We probably want to take this on 3.1 branch. Need to work out if its safe first though.