Last Comment Bug 758747 - Compose window is broken with Personas
: Compose window is broken with Personas
Status: RESOLVED FIXED
: regression
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: Thunderbird 16.0
Assigned To: Mark Banner (:standard8)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-25 12:48 PDT by Jonathan Protzenko [:protz]
Modified: 2012-06-19 13:45 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
fixed


Attachments
The fix (1.98 KB, patch)
2012-06-19 01:44 PDT, Mark Banner (:standard8)
mconley: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Jonathan Protzenko [:protz] 2012-05-25 12:48:47 PDT
Timestamp: 05/25/2012 09:43:33 PM
Error: TypeError: footer is null
Source File: resource:///modules/LightweightThemeConsumer.jsm
Line: 60

This may be related to the fact that I'm unable to use the Insert and Format menu entries.

When we initially checked in support for personas in the composition window, I remember struggling with a similar error. The reason was accessing a XBL binding at script load-time (not page load-time). It would trigger evaluation of all XBL bindings, thus triggering the Persona code before the page DOM was complete.

The XBL binding was simply gComposeBundle, which was queried at script load-time. Maybe someone reintroduced this by accident?
Comment 1 :aceman 2012-05-30 06:53:23 PDT
I confirm seeing this error too but I didn't see the relation to the compose window. I can check that out.
Comment 2 Mark Banner (:standard8) 2012-05-30 07:56:52 PDT
You can debug this sort of issue by putting the statement "debugger;" in around line 60 of that LightweightThemeConsumer.jsm, and then you'll get something like:

------------------------------------------------------------------------
Hit JavaScript "debugger" keyword. JS call stack...
0 anonymous([object Object]) ["resource://gre/modules/LightweightThemeConsumer.jsm":61]
    this = [object Object]
1 LightweightThemeConsumer() ["resource://gre/modules/LightweightThemeConsumer.jsm":17]
    this = [object Object]
2 <TOP LEVEL> ["<unknown>":0]
    <failed to get 'this' value>
3 () ["chrome://global/content/bindings/general.xml":74]
    this = [object XULElement @ 0x12fd58660 (native @ 0x12fe74af0)]
4 <TOP LEVEL> ["chrome://messenger/content/messengercompose/cloudAttachmentLinkManager.js":500]
    this = [object ChromeWindow @ 0x128aa3be0 (native @ 0x11e925480)]
------------------------------------------------------------------------

and bingo!

There's actually two places that are at issue for us:

http://hg.mozilla.org/comm-central/annotate/27498a828ee9/mail/components/compose/content/cloudAttachmentLinkManager.js#l499
http://hg.mozilla.org/comm-central/annotate/27498a828ee9/mail/components/compose/content/bigFileObserver.js#l289

commenting those out "fix" the compose window with personas.

What's happening is the document.documentElement is trying to touch the DOM too early, and causing things to be initialised before they are ready.

I've not thought of a way around this yet without altering the compose code - the problem is that we really want to set up these event listeners in the window onload event but that may or may not happen before the ComposeLoad gets called from the window's onload, and the functions called from ComposeLoad are the ones that trigger the compose-window-init event.
Comment 3 Mark Banner (:standard8) 2012-06-11 03:41:58 PDT
David/Mike, is there some other way of hooking up this event that we could use?
Comment 4 David :Bienvenu 2012-06-11 07:45:42 PDT
I would think there is an easier way, but perhaps we could use compose listener events like extensions do. You register a compose listener event upon getting a compose-window-init event, and then listen for ComposeFieldsReady or ComposeBodyReady. At that point, you're pretty much guaranteed that the dom is ready, though maybe it's too late in the process; Or we could add a new compose-window- event, for when the dom is complete.
Comment 5 Mark Banner (:standard8) 2012-06-19 01:44:02 PDT
Created attachment 634321 [details] [diff] [review]
The fix

Originally I thought that changing to window.addEventListener was the way to fix this, but I hadn't thought about changing the final parameter, changing that to true makes the window version work, fixes the lightweight theme issue and we still pass the cloudfile mozmill tests.
Comment 6 Mike Conley (:mconley) - (needinfo me!) 2012-06-19 07:56:49 PDT
Comment on attachment 634321 [details] [diff] [review]
The fix

Review of attachment 634321 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, and fixes the issue. Cloudfile tests como

Do we know *why* this fixes the issue?
Comment 7 Mark Banner (:standard8) 2012-06-19 08:03:25 PDT
(In reply to Mike Conley (:mconley) from comment #6)
> Do we know *why* this fixes the issue?

See comment 0 and comment 2.
Comment 8 Mike Conley (:mconley) - (needinfo me!) 2012-06-19 08:07:14 PDT
(In reply to Mark Banner (:standard8) from comment #7)
> (In reply to Mike Conley (:mconley) from comment #6)
> > Do we know *why* this fixes the issue?
> 
> See comment 0 and comment 2.

Right - but I'm unsure why the useCapture argument being set to true solves the problem. *sigh*, time to read http://www.w3.org/TR/DOM-Level-3-Events/#event-flow I guess.
Comment 9 Mark Banner (:standard8) 2012-06-19 08:20:23 PDT
Checked in:

https://hg.mozilla.org/comm-central/rev/09ee92ece7f2
Comment 10 Mark Banner (:standard8) 2012-06-19 13:06:32 PDT
Comment on attachment 634321 [details] [diff] [review]
The fix

[Triage Comment]
I'm going to accelerate this onto aurora/beta so that we can get it fixed for the next release.

Note You need to log in before you can comment on or make changes to this bug.