687332 - Move various onfoo event listeners off of DOM objects and into event listener managers

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: FileReader

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: XHR

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: WebSockets

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: EventSource

Comment 5

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

Comment on attachment 560779 [details] [diff] [review]
XHR


> class nsXHREventTarget : public nsDOMEventTargetWrapperCache,
>                          public nsIXMLHttpRequestEventTarget
> {
> public:
>   virtual ~nsXHREventTarget() {}
>   NS_DECL_ISUPPORTS_INHERITED
>-  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(nsXHREventTarget,
>-                                           nsDOMEventTargetWrapperCache)
I think it would be easier, and less error-prone to keep nsXHREventTarget
CCable. That way when someone adds member variables to it, it is easy to
CC the variable.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Olli Pettay [:smaug] from comment #5)
> Comment on attachment 560779 [details] [diff] [review]
> XHR
> 
> 
> > class nsXHREventTarget : public nsDOMEventTargetWrapperCache,
> >                          public nsIXMLHttpRequestEventTarget
> > {
> > public:
> >   virtual ~nsXHREventTarget() {}
> >   NS_DECL_ISUPPORTS_INHERITED
> >-  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(nsXHREventTarget,
> >-                                           nsDOMEventTargetWrapperCache)
> I think it would be easier, and less error-prone to keep nsXHREventTarget
> CCable. That way when someone adds member variables to it, it is easy to
> CC the variable.

Ok, I'll leave it in with a comment explaining why it's still there.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Unfortunately these patches break several tests that expect to pass in something implementing nsIDOMEventListener.  I'm going to file bugs on those and have them block this.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Also, there appears to be an obnoxious compiler issue here.  nsWrapperCache::GetWrapper is implemented as an inline function in xpcpublic.h, and in opt builds on GCC it looks like if a file that uses GetWrapper doesn't include xpcpublic.h linking will fail (since GCC doesn't appear to generate a non-inline version of GetWrapper).

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Work around issues with opt builds and inline functions

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This also seems to totally break geolocation, for reasons that are currently a mystery to me.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So, you can't set onfoo properties in xpcshell, because nsEventListenerManager::SetJSEventListenerToJsval calls nsJSUtils::GetStaticScriptContext which returns null here.

Is it acceptable to require xpcshell stuff to use addEventListener?

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #11)
> So, you can't set onfoo properties in xpcshell, because
> nsEventListenerManager::SetJSEventListenerToJsval calls
> nsJSUtils::GetStaticScriptContext which returns null here.
> 
> Is it acceptable to require xpcshell stuff to use addEventListener?

smaug says yes via IRC.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Fix geolocation

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Fix toolkit

Comment 15

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

Hmm, this can affect to addons compatibility.

Comment 16

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

(We should remove that possibility to create XHR using createInstance)

Comment 17

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

Kyle, could we actually check mOwner in SetOn##_event.
If we don't have mOwner, wrap the function to nsIDOMEventListener and call normal
addEventListener. That is not a perfect solution, since xhr.onload = l; xhr.removeEventListener("load", l); wouldn't work per spec, but at least that would be visible only
to JS components.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Olli Pettay [:smaug] from comment #17)
> Kyle, could we actually check mOwner in SetOn##_event.
> If we don't have mOwner, wrap the function to nsIDOMEventListener and call
> normal
> addEventListener. That is not a perfect solution, since xhr.onload = l;
> xhr.removeEventListener("load", l); wouldn't work per spec, but at least
> that would be visible only
> to JS components.

Perhaps.  We should investigate how many addons use this.

I'm also going to avoid landing this until after branch so that we have the maximum time possible for people to adjust.

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Test fixes

These are the purely mechanical test fixes.

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

With all dependent bugs fixed and all the patches in this bug applied except the websockets one, this passes all tests.

The websockets one is causing some weird test timeout that I can't debug locally, so I'm going to punt on that for the moment.

Comment 21

[[e-mail]]

Comment on attachment 561863 [details] [diff] [review]
Fix toolkit

>     // Create a closure
>     var self = this;
>-    this._request.onreadystatechange = function() {
>+    this._request.addEventListener("readystatechange", function() {
>       self.readyStateChange(self);
>-    }
>+    }, false);
Eww, why do we still have these ugly closures all over :-(

this._request.addEventListener("readystatechange", this.readyStateChange.bind(this), false);

Although personally I prefer this way which has worked since the dawn of time:

this._request.addEventListener("readystatechange", this, false);
...
handleEvent: function(aEvent) {
  switch (aEvent.type) {
    case "readystatechange":
      this.readyStateChange();
    // other event types where necessary
  }
},

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to [e-mail] from comment #21)
> Comment on attachment 561863 [details] [diff] [review] [diff] [details] [review]
> Fix toolkit
> 
> >     // Create a closure
> >     var self = this;
> >-    this._request.onreadystatechange = function() {
> >+    this._request.addEventListener("readystatechange", function() {
> >       self.readyStateChange(self);
> >-    }
> >+    }, false);
> Eww, why do we still have these ugly closures all over :-(
> 
> this._request.addEventListener("readystatechange",
> this.readyStateChange.bind(this), false);

Because I'm not going through and fixing those ;-)

> Although personally I prefer this way which has worked since the dawn of
> time:
> 
> this._request.addEventListener("readystatechange", this, false);
> ...
> handleEvent: function(aEvent) {
>   switch (aEvent.type) {
>     case "readystatechange":
>       this.readyStateChange();
>     // other event types where necessary
>   }
> },

That no longer works.

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #22)
> (In reply to [e-mail] from comment #21)
> > Although personally I prefer this way which has worked since the dawn of
> > time:
> > 
> > this._request.addEventListener("readystatechange", this, false);
> > ...
> > handleEvent: function(aEvent) {
> >   switch (aEvent.type) {
> >     case "readystatechange":
> >       this.readyStateChange();
> >     // other event types where necessary
> >   }
> > },
> 
> That no longer works.

Er, that does still work.  What no longer works is

this._request.onreadystatechange = this;

handleEvent: function ...

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/34b49c378a6b
https://hg.mozilla.org/mozilla-central/rev/26f22c7666b3
https://hg.mozilla.org/mozilla-central/rev/aa0657195eff
https://hg.mozilla.org/mozilla-central/rev/d2dbc7ab380e
https://hg.mozilla.org/mozilla-central/rev/4640b44402ac
https://hg.mozilla.org/mozilla-central/rev/867d159c1508
https://hg.mozilla.org/mozilla-central/rev/1f8c38d6e4bf
https://hg.mozilla.org/mozilla-central/rev/acffd2f7d665
https://hg.mozilla.org/mozilla-central/rev/6d59d323116d
https://hg.mozilla.org/mozilla-central/rev/ff191893660f
https://hg.mozilla.org/mozilla-central/rev/69e1b5167409
https://hg.mozilla.org/mozilla-central/rev/1aa8a5876058

Comment 25

[Kent James (:rkent)]

As an addon editor, I was pointed to this bug because it breaks many things.

What I find objectional is that you add this with absolutely no justification anywhere in the bug. Don't you think it would be appropriate to at least say WHY you want to add code that breaks many addons?

Comment 26

[Jorge Villalobos [:jorgev] (he/him)]

I'm certain this will break a whole lot of add-ons, and it would be very difficult for us to tell the acceptable uses of onfoo apart from the 'bad' ones. There needs to be a deprecation period where onfoo still work and a JS error is shown in the console.

Why is this change even necessary?

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Kent James (:rkent) from comment #25)
> As an addon editor, I was pointed to this bug because it breaks many things.
> 
> What I find objectional is that you add this with absolutely no
> justification anywhere in the bug. Don't you think it would be appropriate
> to at least say WHY you want to add code that breaks many addons?

Well, when I filed the bug I didn't realize it was going to break them ;-)

But sure, the reason we want to do this is because it moves us from having multiple different codepaths for handling event listeners on different types of DOM objects towards having one codepath that handles them all.

(In reply to Jorge Villalobos [:jorgev] from comment #26)
> I'm certain this will break a whole lot of add-ons, and it would be very
> difficult for us to tell the acceptable uses of onfoo apart from the 'bad'
> ones. There needs to be a deprecation period where onfoo still work and a JS
> error is shown in the console.

Do you expect that the fallout will be solely due to XHR?  We can special case XHR if we have to.  I don't really want to, but the option is there.

> Why is this change even necessary?

It's not necessary, but it is desirable, for the above reasons.

Comment 28

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I've backed this out for the moment until we sort this out.

https://hg.mozilla.org/mozilla-central/rev/0eb7a5b286bb

Comment 29

[LegNeato]

FWIW I've talked to Jonas about a general deprecation strategy for this type of stuff (something like "must throw a warning for X number of releases, the warning should have the date it will be removed, etc"). I think it's good to simplify code but we need to give some heads up and migration time (I'm not sure where the sweet spot is).

Related, we filed a bug for a generic deprecation warning message we could land all the way even on beta w/o l10n impact (bug 661958).

Comment 30

[Boris Zbarsky [:bzbarsky]]

> It's not necessary, but it is desirable

So...  That can't be the case.  Either this patch is necessary to follow the spec here (and addons are not compatible with what the spec requires the behavior to be) or it's wrong and violates the spec.  One or the other.

Comment 31

[Boris Zbarsky [:bzbarsky]]

For example, the FileReader spec says:

  attribute Function onloadstart;

whereas our IDL required an nsIDOMEventListener.  This caused observable differences in behavior between our code and the spec.  Of course the new code doesn't match what the spec says either, but the spec actually needs changing here so it's compatible with the HTML on* properties.  And then the new code will be what we need to match the spec.

Which is the answer to comment 25: we need to make this change to comply with web standards, and addons will break to the extent that they rely on behavior that violated those web standards...

Comment 32

[Andrew Williamson [:eviljeff]]

(In reply to Boris Zbarsky (:bz) from comment #31)
> Which is the answer to comment 25: we need to make this change to comply
> with web standards, and addons will break to the extent that they rely on
> behavior that violated those web standards...

I don't think that's fair.  Even the w3c spec draft gives an example which uses on* properties[1].  Also the xhr changes will specifically affect addon modules and components, which don't need to particularly web standard compliant as they're not exposed to the web.

[1] http://www.w3.org/TR/XMLHttpRequest/#introduction

Comment 33

[Boris Zbarsky [:bzbarsky]]

> I don't think that's fair.

Fair in what sense?  It's just a statement of fact....

We're not making on* properties not work.  We're just making it impossible to assign non-functions to them and making it impossible to assign functions not associated with a Window to them.  The former is required by the spec.  The latter is an implementation constraint that we have for the moment if we're going to do the former; it's obviously not needed for spec compliance because that problem can never arise in a web context.  Fixing that implementation constraint would be good for sure; it's not clear how to do that yet.  Comment 17 has one proposal tat might work, maybe.

> Also the xhr changes will specifically affect addon modules and components

They affect all uses of on* on XHR, including web-facing ones.  It's trivial to write web code that used to work in Gecko before this change, and won't work afterward.

> which don't need to particularly web standard compliant as they're not exposed to the
> web.

If they're using web technologies, then by default web standards apply.  It's not like we have two different XHR objects around.

Comment 34

[Andrew Williamson [:eviljeff]]

(In reply to Boris Zbarsky (:bz) from comment #33)
> > I don't think that's fair.
> 
> Fair in what sense?  It's just a statement of fact....

Not fair in the sense that the implication is that its the addon developers fault for not following 'standards', even if they have followed the W3C spec example, but put the code in a module rather than window.

> We're not making on* properties not work.  We're just making it impossible
> to assign non-functions to them and making it impossible to assign functions
> not associated with a Window to them.  The former is required by the spec. 
> The latter is an implementation constraint that we have for the moment if
> we're going to do the former; it's obviously not needed for spec compliance
> because that problem can never arise in a web context.  Fixing that
> implementation constraint would be good for sure; it's not clear how to do
> that yet.  Comment 17 has one proposal tat might work, maybe.

In the case of JSMs the changes /will/ make on* properties not work as there isn't a window.  If the reason is an implementation one rather than spec compliance then, imo, the implementation as it stands is inadequate.

> > Also the xhr changes will specifically affect addon modules and components
> 
> They affect all uses of on* on XHR, including web-facing ones.  It's trivial
> to write web code that used to work in Gecko before this change, and won't
> work afterward.
> 
> > which don't need to particularly web standard compliant as they're not exposed to the
> > web.
> 
> If they're using web technologies, then by default web standards apply. 
> It's not like we have two different XHR objects around.

There may as well be two different XHR objects if a function in an overlay script will be able to assign functions to on* on the XHR object (and work) and the same function in a JSM won't.  That's what will specifically affect addon modules.

Comment 35

[Boris Zbarsky [:bzbarsky]]

> that the implication is that its the addon developers fault

I wasn't trying to imply anything or assign fault; simply to describe the precise scope of the problem we have.

> then, imo, the implementation as it stands is inadequate.

I thought we'd all agreed on that, moved past that point, and were figuring out how to fix the implementation at this point.  Why are we still discussing this part?

Comment 36

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The tricky bit here is not storing the event listeners on the DOM object anymore.  I don't see any easy way to avoid that while preserving the current behavior in component scopes, and that was the whole reason I wanted to do this in the first place :-/

Comment 37

[Mike Kaply [:mkaply]]

Is there something simple you can do to detect this behavior before even attempting to fix it so that add-on developers could know the problem in advance?

So basically, next version tells add-on developers of the problem (but no code changes).

In reading your blog post, I honestly couldn't exactly tell when I would even run into this problem.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Olli, can we remove the nsIScriptContext dependency of nsJSEventListener?  Or come up with an nsIScriptContext in component scope?

Comment 39

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

Kyle, did you try comment 17? That might be easy, and should be quite ok, IMHO.

Other option is to make nsJSContext::CallEventHandler some kind of static method, which doesn't
rely on nsIScriptContext. Though, need to make sure then that we have a reasonable JSContext* kept
alive when HandleEvent is called.

Comment 40

[Giorgio Maone [:ma1]]

(In reply to Boris Zbarsky (:bz) from comment #33)

> We're not making on* properties not work.  We're just making it impossible
> to assign non-functions to them and making it impossible to assign functions
> not associated with a Window to them.  The former is required by the spec. 
> The latter is an implementation constraint that we have for the moment if
> we're going to do the former; it's obviously not needed for spec compliance
> because that problem can never arise in a web context.

What about Web Workers?

Comment 41

[Boris Zbarsky [:bzbarsky]]

> What about Web Workers?

I believe things should continue to work fine there, but worth double-checking, yes.  ;)

Comment 42

[Jorge Villalobos [:jorgev] (he/him)]

This bug needs its milestone updated, right? I'm looking into breaking changes in Firefox 10 and this bug appears as a candidate at the moment.

Comment 43

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 1: Explicitly keep track of which event listener holds the event handler

This will allow .onfoo = function; to keep working in non-window scopes.

Comment 44

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

Comment on attachment 656328 [details] [diff] [review]
Part 1: Explicitly keep track of which event listener holds the event handler

># HG changeset patch
># Parent ccfe67baa9dde3b852f9f2deebe58cc105bd8a3b
># User Kyle Huey <[e-mail]>
>diff --git a/content/events/src/nsEventListenerManager.cpp b/content/events/src/nsEventListenerManager.cpp
>--- a/content/events/src/nsEventListenerManager.cpp
>+++ b/content/events/src/nsEventListenerManager.cpp
>@@ -180,44 +180,48 @@ nsEventListenerManager::GetInnerWindowFo
> 
>   return nullptr;
> }
> 
> void
> nsEventListenerManager::AddEventListener(nsIDOMEventListener *aListener,
>                                          uint32_t aType,
>                                          nsIAtom* aTypeAtom,
>-                                         int32_t aFlags)
>+                                         int32_t aFlags,
>+                                         bool aHandler)
> {
>   NS_ABORT_IF_FALSE(aType && aTypeAtom, "Missing type");
> 
>   if (!aListener) {
>     return;
>   }
> 
>   nsRefPtr<nsIDOMEventListener> kungFuDeathGrip = aListener;
> 
>   nsListenerStruct* ls;
>   uint32_t count = mListeners.Length();
>   for (uint32_t i = 0; i < count; i++) {
>     ls = &mListeners.ElementAt(i);
>-    if (ls->mListener == aListener && ls->mFlags == aFlags &&
>+    if (ls->mListener == aListener &&
>+        ls->mListenerIsHandler == ls->mListenerIsHandler &&
Er, what?

>@@ -39,16 +39,17 @@ typedef enum
> 
> struct nsListenerStruct
> {
>   nsRefPtr<nsIDOMEventListener> mListener;
>   uint32_t                      mEventType;
>   nsCOMPtr<nsIAtom>             mTypeAtom;
>   uint16_t                      mFlags;
>   uint8_t                       mListenerType;
>+  bool                          mListenerIsHandler;
>   bool                          mHandlerIsString;
Hey, while you're here, could you
change the member variables to such that even Win compiler makes the struct as small as possible.
Is it
uint32_t mFlags:16;
uint32_t mListenerType:8;
uint32_t mListenerIsHandler:1;
uint32_t mHandlerIsString:1;

Comment 45

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 1: Explicitly keep track of which event listener holds the event handler

Comment 46

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 2: Move event handlers into the event listener manager

This is a pretty big patch, but it's mostly boilerplate.

This keeps xhr.onfoo = function() { } in non-DOM scopes, but it does prevent xhr.onfoo = { handleEvent: function() { } }; from working everywhere, so it brings us in line with the spec.

Comment 47

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

Comment on attachment 656638 [details] [diff] [review]
Part 1: Explicitly keep track of which event listener holds the event handler


>+++ b/content/events/src/nsEventListenerManager.h
>@@ -39,16 +39,17 @@ typedef enum
> 
> struct nsListenerStruct
> {
>   nsRefPtr<nsIDOMEventListener> mListener;
>   uint32_t                      mEventType;
>   nsCOMPtr<nsIAtom>             mTypeAtom;
>   uint16_t                      mFlags;
>   uint8_t                       mListenerType;
>+  bool                          mListenerIsHandler;
>   bool                          mHandlerIsString;

So, while you're here, could you make sure Windows compiler actually
does something reasonable here and keeps mFlags, mListenerType, mListenerIsHandler and mHandlerIsString
within one word.
Feel free to change types to uint32_t ... :8 or :1 if needed.

Comment 48

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

Comment on attachment 656639 [details] [diff] [review]
Part 2: Move event handlers into the event listener manager

There shouldn't be any need for these changes. 
>+nsDOMEventTargetHelper::SetEventHandler(nsIAtom* aType,
>+                                        JSContext* aCx,
>+                                        const JS::Value& aValue)
> {
>-  if (aCurrent) {
>-    RemoveEventListener(aType, aCurrent, false);
>-    aCurrent = nullptr;
>+  nsEventListenerManager* elm = GetListenerManager(true);
>+  NS_ENSURE_STATE(elm);
This means OOM, so there shouldn't be need for the NS_ENSURE_STATE




> nsEventListenerManager::SetEventHandlerInternal(nsIScriptContext *aContext,
>+                                                JSContext* aCx,
>                                                 JSObject* aScopeObject,
>                                                 nsIAtom* aName,
>                                                 JSObject *aHandler,
>                                                 bool aPermitUntrustedEvents,
>                                                 nsListenerStruct **aListenerStruct)
> {
>+  NS_ASSERTION(aContext || aCx, "Must have one or the other!");
>+
>   nsresult rv = NS_OK;
>   uint32_t eventType = nsContentUtils::GetEventId(aName);
>   nsListenerStruct* ls = FindEventHandler(eventType, aName);
> 
>   if (!ls) {
>     // If we didn't find a script listener or no listeners existed
>     // create and add a new one.
>-    nsCOMPtr<nsIJSEventListener> scriptListener;
>-    rv = NS_NewJSEventListener(aContext, aScopeObject, mTarget, aName,
>-                               aHandler, getter_AddRefs(scriptListener));
>+    const uint32_t flags = NS_EVENT_FLAG_BUBBLE |
>+                           (aContext ? NS_PRIV_EVENT_FLAG_SCRIPT : 0);
>+    nsCOMPtr<nsIDOMEventListener> listener;
>+
>+    if (aContext) {
>+      nsCOMPtr<nsIJSEventListener> scriptListener;
>+      rv = NS_NewJSEventListener(aContext, aScopeObject, mTarget, aName,
>+                                 aHandler, getter_AddRefs(scriptListener));
>+      listener = scriptListener.forget();
>+    }
>+    else {
} else {



>+      // If we don't have a script context, we're setting an event handler from
>+      // a component or other odd scope.  Ask XPConnect if it can make us an
>+      // nsIDOMEventListener.
>+      rv = nsContentUtils::XPConnect()->WrapJS(aCx,
>+                                               aHandler,
>+                                               NS_GET_IID(nsIDOMEventListener),
>+                                               getter_AddRefs(listener));
>+    }
Ah, this way... should be ok.


>+    if (scriptListener) {
>+      scriptListener->SetHandler(aHandler);
>+    }
>+    else {
} else {

>@@ -21,17 +21,17 @@ interface nsIDOMScreen : nsIDOMEventTarg
> 
>   /**
>    * Returns the current screen orientation.
>    * Can be: landscape-primary, landscape-secondary,
>    *         portrait-primary or portrait-secondary.
>    */
>   readonly attribute DOMString       mozOrientation;
> 
>-  attribute nsIDOMEventListener      onmozorientationchange;
>+  [implicit_jscontext] attribute jsval      onmozorientationchange;
A bit less space after jsval should be enough

>+++ b/js/xpconnect/src/nsXPConnect.cpp
>@@ -1423,16 +1423,20 @@ nsXPConnect::GetNativeOfWrapper(JSContex
>         // FIXME: Provide a fast non-refcounting way to get the canonical
>         //        nsISupports from the proxy.
>         nsISupports *supports =
>             static_cast<nsISupports*>(js::GetProxyPrivate(aJSObj).toPrivate());
>         nsCOMPtr<nsISupports> canonical = do_QueryInterface(supports);
>         return canonical.get();
>     }
> 
>+    nsISupports* supports = nullptr;
>+    if (mozilla::dom::UnwrapDOMObjectToISupports(aJSObj, supports))
>+        return supports;
>+
Someone else should review this.


Make sure this all compiles also in b2g context.

Comment 49

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Olli Pettay [:smaug] from comment #47)
> Comment on attachment 656638 [details] [diff] [review]
> Part 1: Explicitly keep track of which event listener holds the event handler
> 
> 
> >+++ b/content/events/src/nsEventListenerManager.h
> >@@ -39,16 +39,17 @@ typedef enum
> > 
> > struct nsListenerStruct
> > {
> >   nsRefPtr<nsIDOMEventListener> mListener;
> >   uint32_t                      mEventType;
> >   nsCOMPtr<nsIAtom>             mTypeAtom;
> >   uint16_t                      mFlags;
> >   uint8_t                       mListenerType;
> >+  bool                          mListenerIsHandler;
> >   bool                          mHandlerIsString;
> 
> So, while you're here, could you make sure Windows compiler actually
> does something reasonable here and keeps mFlags, mListenerType,
> mListenerIsHandler and mHandlerIsString
> within one word.
> Feel free to change types to uint32_t ... :8 or :1 if needed.

The Microsoft compiler uses a byte for each of the bools.  This results in the struct being a word longer than necessary because the last bool makes the struct 17 bytes long.  jemalloc then rounds this up to 32 bytes :-(  I'll fix this, but in a followup.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 656639 [details] [diff] [review]
Part 2: Move event handlers into the event listener manager

r=me on the nsXPConnect change.

Comment 51

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/9baa8a16f944
https://hg.mozilla.org/mozilla-central/rev/0a1f4d81635a

Comment 54

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/e3f700f4d0cb
https://hg.mozilla.org/mozilla-central/rev/fcc533f691e9

Comment 55

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So, a recap for everyone on what we did.

onfoo = function() { }; still works in component, JSM, etc scopes.  This was not the case when this landed originally, and is why this was backed out.

What we did change (in order of most visible to least visible):

onfoo = { handleEvent: function() { } }; does not work anymore, in any scope.  This aligns us with the spec and with the behavior of other (non-WebKit) browsers.

Exceptions thrown from onfoo handlers are no longer eaten by XPConnect.

We now get event handlers ordered correctly.  That is,

addEventListener("foo", f1);
onfoo = notcalled;
addEventListener("foo", f3);
onfoo = f2;

calls f1, f2, then f3 (it used to call f1, f3, then f2).

Comment 56

[Mike Kaply [:mkaply]]

> onfoo = { handleEvent: function() { } }; does not work anymore, in any scope.  This aligns us with the spec and with the behavior of other (non-WebKit) browsers.

When you say "does not work", what does that mean?

Will there be an error in the console?

Is this a common pattern that we'll need to search add-ons for?

You mentioned non-WebKit browsers. Is this yet another compatibility issue we'll have with webkit?

Comment 57

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Michael Kaply (mkaply) from comment #56)
> > onfoo = { handleEvent: function() { } }; does not work anymore, in any scope.  This aligns us with the spec and with the behavior of other (non-WebKit) browsers.
> 
> When you say "does not work", what does that mean?

Because an object implementing handleEvent is not callable, and onfoo attributes are [TreatNonCallableAsNull], .onfoo = { handleEvent: function() { } } has the same effect as .onfoo = null;

This aligns us with what Opera does today and what IE effectively does today (IE allows you to set the property but doesn't actually fire the event listener if you do).

> Will there be an error in the console?

No.

> Is this a common pattern that we'll need to search add-ons for?

I don't know.

> You mentioned non-WebKit browsers. Is this yet another compatibility issue
> we'll have with webkit?

Until WebKit aligns with the spec, yes.  I filed https://bugs.webkit.org/show_bug.cgi?id=95566.

Comment 58

[Kohei Yoshino]

https://blog.mozilla.org/addons/2012/12/28/compatibility-for-firefox-18/
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_18

Comment 59

[Boris Zbarsky [:bzbarsky]]

Kohei Yoshino, it's perhaps worth pointing out that the change in this bug only affects DOM objects that are not DOM nodes.  More precisely, it makes various non-node objects have the behavior that nodes already had.

Comment 60

[Boris Zbarsky [:bzbarsky]]

And in particular, the "move off" phrasing makes it sound like the properties got removed, which is not the case.  What was moved is where the values are _stored_.  The script-visible change is that setting on* properties to objects with a handleEvent attribute will no longer work on any DOM objects, just like it used to not work for nodes.

I've fixed the devmo documentation accordingly to make it clear what really changed and what the behavior was before, but I can't edit the blog post...

Comment 61

[Kris Maglione [:kmag]]

There isn't anything especially inaccurate in the blog post, except that it uses this bug's slightly confusing summary. I think it would be better to link the devmo page than this bug, though, since there was a lot of noise here early on that's since been cleared up.

Comment 62

[Boris Zbarsky [:bzbarsky]]

The blog post only uses part of the summary, making it say something completely different from what the actual summary says...

Comment 63

[Kris Maglione [:kmag]]

I doubt that the elision makes any difference to anyone not intimately familiar with Gecko internals. The bug summary is written from the perspective of someone interested in code cleanup. The summary in the blog post should reflect the implications for add-on developers.

Comment 64

[Kohei Yoshino]

(In reply to Boris Zbarsky (:bz) from comment #60)
> I've fixed the devmo documentation accordingly to make it clear what really
> changed and what the behavior was before

Thank you! I just updated the Japanese translation.
(Actually the article was written in Japanese first.)

(In reply to Kris Maglione [:kmag] from comment #63)
> The summary in the blog post should reflect the implications for add-on developers

+1

Comment 65

[Boris Zbarsky [:bzbarsky]]

> The summary in the blog post should reflect the implications for add-on developers

Yes, agreed.  Who can change the blog post?

Comment 66

[Kris Maglione [:kmag]]

Jorge is the only one who can update the blog post. I sent him an email since he seems to have stopped following this bug.

Comment 67

[Kohei Yoshino]

The Add-ons Blog post has been updated to point the MDN article.