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

RESOLVED FIXED in mozilla18

Status

()

Core
DOM: Events
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

(Blocks: 1 bug, {addon-compat, dev-doc-needed})

unspecified
mozilla18
addon-compat, dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 9 obsolete attachments)

4.86 KB, patch
smaug
: review+
Details | Diff | Splinter Review
173.81 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
Created attachment 560778 [details] [diff] [review]
FileReader
Attachment #560778 - Flags: review?(Olli.Pettay)
Created attachment 560779 [details] [diff] [review]
XHR
Attachment #560779 - Flags: review?(Olli.Pettay)
Created attachment 560780 [details] [diff] [review]
WebSockets
Attachment #560780 - Flags: review?(Olli.Pettay)
Created attachment 560781 [details] [diff] [review]
EventSource
Attachment #560781 - Flags: review?(Olli.Pettay)

Updated

6 years ago
Attachment #560778 - Flags: review?(Olli.Pettay) → review+

Comment 5

6 years ago
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.
Attachment #560779 - Flags: review?(Olli.Pettay) → review+

Updated

6 years ago
Attachment #560780 - Flags: review?(Olli.Pettay) → review+

Updated

6 years ago
Attachment #560781 - Flags: review?(Olli.Pettay) → review+
(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.
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.
Depends on: 688416
Depends on: 688417
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).
Created attachment 561733 [details] [diff] [review]
Work around issues with opt builds and inline functions
This also seems to totally break geolocation, for reasons that are currently a mystery to me.
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?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Created attachment 561769 [details] [diff] [review]
Fix geolocation
Attachment #561769 - Flags: review?(doug.turner)
Created attachment 561863 [details] [diff] [review]
Fix toolkit
Attachment #561863 - Flags: review?
Attachment #561863 - Flags: review? → review?(robert.bugzilla)

Updated

6 years ago
Attachment #561769 - Flags: review?(doug.turner) → review+
Hmm, this can affect to addons compatibility.
(We should remove that possibility to create XHR using createInstance)
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.
(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.
Keywords: addon-compat, dev-doc-needed
Depends on: 688776
Created attachment 562072 [details] [diff] [review]
Test fixes

These are the purely mechanical test fixes.
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.
Attachment #560780 - Attachment is obsolete: true
Attachment #561863 - Flags: review?(robert.bugzilla) → review+
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
  }
},
(In reply to neil@parkwaycc.co.uk 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.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #22)
> (In reply to neil@parkwaycc.co.uk 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 ...
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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10

Comment 25

6 years ago
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?
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?
(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.
I've backed this out for the moment until we sort this out.

https://hg.mozilla.org/mozilla-central/rev/0eb7a5b286bb
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 29

6 years ago
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).
> 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.
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...
(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
> 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.
(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.
> 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?
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 :-/
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.
Olli, can we remove the nsIScriptContext dependency of nsJSEventListener?  Or come up with an nsIScriptContext in component scope?
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.
(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?
> What about Web Workers?

I believe things should continue to work fine there, but worth double-checking, yes.  ;)
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.
Blocks: 713598

Updated

5 years ago
Blocks: 580070
Blocks: 739044
Status: REOPENED → ASSIGNED
Target Milestone: mozilla10 → ---
Created attachment 656328 [details] [diff] [review]
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.
Attachment #560778 - Attachment is obsolete: true
Attachment #560779 - Attachment is obsolete: true
Attachment #560781 - Attachment is obsolete: true
Attachment #561733 - Attachment is obsolete: true
Attachment #561769 - Attachment is obsolete: true
Attachment #561863 - Attachment is obsolete: true
Attachment #562072 - Attachment is obsolete: true
Attachment #656328 - Flags: review?(bugs)
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 <khuey@kylehuey.com>
>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;
Attachment #656328 - Flags: review?(bugs) → review-
Created attachment 656638 [details] [diff] [review]
Part 1: Explicitly keep track of which event listener holds the event handler
Attachment #656328 - Attachment is obsolete: true
Attachment #656638 - Flags: review?(bugs)
Created attachment 656639 [details] [diff] [review]
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.
Attachment #656639 - Flags: review?(bugs)
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.
Attachment #656638 - Flags: review?(bugs) → review+
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.
Attachment #656639 - Flags: review?(bugs) → review+
(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 on attachment 656639 [details] [diff] [review]
Part 2: Move event handlers into the event listener manager

r=me on the nsXPConnect change.
Attachment #656639 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9baa8a16f944
https://hg.mozilla.org/mozilla-central/rev/0a1f4d81635a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Updated

5 years ago
Duplicate of this bug: 713598

Updated

5 years ago
Duplicate of this bug: 739044
https://hg.mozilla.org/mozilla-central/rev/e3f700f4d0cb
https://hg.mozilla.org/mozilla-central/rev/fcc533f691e9
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).
> 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?
(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.
Depends on: 787512

Updated

5 years ago
Blocks: 787514
Blocks: 790263

Updated

5 years ago
Depends on: 795256

Updated

5 years ago
No longer depends on: 795256
https://blog.mozilla.org/addons/2012/12/28/compatibility-for-firefox-18/
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_18
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.
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...
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.
The blog post only uses part of the summary, making it say something completely different from what the actual summary says...
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.
(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
> The summary in the blog post should reflect the implications for add-on developers

Yes, agreed.  Who can change the blog post?
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.
The Add-ons Blog post has been updated to point the MDN article.
Depends on: 838721
You need to log in before you can comment on or make changes to this bug.