603535 - nsIEventListenerService.getEventTargetChainFor failure on TM nightly

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

Using bsmedberg's "AeroWindowTitle" add-on (https://addons.mozilla.org/en-US/firefox/addon/221514/) on latest TM nightly (20101011), I get the following exception on the error console:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIEventListenerService.getEventTargetChainFor]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: jar:file:///C:/Users/Felipe/AppData/Roaming/Mozilla/Firefox/Profiles/zb2xkbgt.default/extensions/[e-mail]!/aero-overlay.xul :: anonymous :: line 111"  data: no]

That line in question from aero-overlay.xul is:

109    var els = Components.classes["@mozilla.org/eventlistenerservice;1"].
110      getService(Components.interfaces.nsIEventListenerService);
111    var lastListener = els.getEventTargetChainFor(window).pop().
112      QueryInterface(Components.interfaces.nsIDOMNSEventTarget);

Comment 1

[Boris Zbarsky [:bzbarsky]]

In nsEventListenerService::GetEventTargetChainFor, we call nsEventDispatcher::Dispatch.  This does:

542       if (!targetEtci->IsValid()) {
543         nsEventTargetChainItem::Destroy(pool.GetPool(), targetEtci);
544         return NS_ERROR_FAILURE;
545       }

in our case, IsValid() tests false: targetEtci has a null mTarget.

And the reason for that is that is that the event target that got passed to nsEventListenerService::GetEventTargetChainFor is the _inner_ window.  So this frame:

#1  0x000000010074a75c in nsEventTargetChainItem::nsEventTargetChainItem (this=0x10689d7c0, aTarget=0x11d0b34d8, aChild=0x0) at /Users/bzbarsky/mozilla/tracemonkey/mozilla/content/events/src/nsEventDispatcher.cpp:258
258       mTarget = aTarget->GetTargetForEventTargetChain();

which calls this:

#0  nsGlobalWindow::GetTargetForEventTargetChain (this=0x11d0b3450) at nsGlobalWindow.h:376
376         return static_cast<nsPIDOMEventTarget*>(GetCurrentInnerWindowInternal());

ends up setting mTarget to null.
Why are we ending up with an inner window here?  

As a separate question, should GetTargetForEventTargetChain() be made to work on inner windows?

Comment 2

[Boris Zbarsky [:bzbarsky]]

ccing smaug for that last question.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

I don't quite understand the question. How is GetTargetForEventTargetChain()
not working atm?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Ah, I see.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #1)
> 
> ends up setting mTarget to null.
> Why are we ending up with an inner window here?  
However, this is more interesting question.

Comment 6

[Boris Zbarsky [:bzbarsky]]

>> Why are we ending up with an inner window here?  
>However, this is more interesting question.
Sure; that was the actual behavior change.  ;)

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

This is one of the changes that was required for brain transplants. Because we don't have an XPCWrappedNative for the outer window anymore, we can't pass the outer window when we're passed 'window' from JS. This means that places that depend on having an outer window need to outerize at some point (at the same time, they can assume that they get an inner window).

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

So when should we use inner window and when outer?
I'm don't know how "brain transplants" work.

Currently inner window is used when creating event target chain, but
event.target/.currentTarget etc. return the outer window.

Comment 9

[Boris Zbarsky [:bzbarsky]]

That part is all fine.

What Blake is saying is that any time a Window is passed from JS into C++ via XPConnect what you will end up with on the C++ side is the inner window.  It used to be the outer window before he brain transplants change.  So now the argument to GetEventTargetChainFor is an inner window, not an outer one, and the inner window ends up getting passed to Dispatch().  The event code right now works only if the target passed to Dispatch() is either not a window or is an outer window.

Blake, is there no way we can change xpconnect unwrapping to preserve the old behavior?  It's easy to fix the event code here, but this change in what window we get is kinda scary...

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

So does this fix the problem?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yep.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

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

Where should this patch land?
To TM? If so, could someone who has TM branch locally
land it there.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

I guess I could land this to m-c.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

oops, this wasn't reviewed yet :)

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/ba9f2a449652