Last Comment Bug 687332 - Move various onfoo event listeners off of DOM objects and into event listener managers
: Move various onfoo event listeners off of DOM objects and into event listener...
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla18
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
: 713598 739044 (view as bug list)
Depends on: 688416 688417 688776 787512 838721
Blocks: ParisBindings 713598 739044 787514 790263
  Show dependency treegraph
 
Reported: 2011-09-18 05:07 PDT by Kyle Huey [:khuey] (khuey@mozilla.com)
Modified: 2013-02-06 10:08 PST (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
FileReader (13.50 KB, patch)
2011-09-18 05:08 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
bugs: review+
Details | Diff | Review
XHR (17.29 KB, patch)
2011-09-18 05:08 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
bugs: review+
Details | Diff | Review
WebSockets (6.22 KB, patch)
2011-09-18 05:10 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
bugs: review+
Details | Diff | Review
EventSource (5.22 KB, patch)
2011-09-18 05:10 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
bugs: review+
Details | Diff | Review
Work around issues with opt builds and inline functions (2.18 KB, patch)
2011-09-22 07:41 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Review
Fix geolocation (1.70 KB, patch)
2011-09-22 09:10 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
dougt: review+
Details | Diff | Review
Fix toolkit (7.57 KB, patch)
2011-09-22 13:31 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
robert.strong.bugs: review+
Details | Diff | Review
Test fixes (6.97 KB, patch)
2011-09-23 10:05 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Review
Part 1: Explicitly keep track of which event listener holds the event handler (4.87 KB, patch)
2012-08-28 22:07 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
bugs: review-
Details | Diff | Review
Part 1: Explicitly keep track of which event listener holds the event handler (4.86 KB, patch)
2012-08-29 15:20 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
bugs: review+
Details | Diff | Review
Part 2: Move event handlers into the event listener manager (173.81 KB, patch)
2012-08-29 15:21 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
bugs: review+
bzbarsky: review+
Details | Diff | Review

Description Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-18 05:07:40 PDT

    
Comment 1 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-18 05:08:05 PDT
Created attachment 560778 [details] [diff] [review]
FileReader
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-18 05:08:58 PDT
Created attachment 560779 [details] [diff] [review]
XHR
Comment 3 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-18 05:10:14 PDT
Created attachment 560780 [details] [diff] [review]
WebSockets
Comment 4 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-18 05:10:41 PDT
Created attachment 560781 [details] [diff] [review]
EventSource
Comment 5 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-09-21 01:31:53 PDT
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 [:khuey] (khuey@mozilla.com) 2011-09-21 08:10:25 PDT
(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 [:khuey] (khuey@mozilla.com) 2011-09-22 05:00:31 PDT
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 [:khuey] (khuey@mozilla.com) 2011-09-22 06:50:16 PDT
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 [:khuey] (khuey@mozilla.com) 2011-09-22 07:41:06 PDT
Created attachment 561733 [details] [diff] [review]
Work around issues with opt builds and inline functions
Comment 10 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-22 08:31:45 PDT
This also seems to totally break geolocation, for reasons that are currently a mystery to me.
Comment 11 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-22 08:47:28 PDT
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 [:khuey] (khuey@mozilla.com) 2011-09-22 08:54:28 PDT
(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.
Comment 13 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-22 09:10:41 PDT
Created attachment 561769 [details] [diff] [review]
Fix geolocation
Comment 14 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-22 13:31:49 PDT
Created attachment 561863 [details] [diff] [review]
Fix toolkit
Comment 15 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-09-22 23:34:15 PDT
Hmm, this can affect to addons compatibility.
Comment 16 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-09-22 23:34:51 PDT
(We should remove that possibility to create XHR using createInstance)
Comment 17 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-09-23 00:05:24 PDT
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 [:khuey] (khuey@mozilla.com) 2011-09-23 09:08:17 PDT
(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 [:khuey] (khuey@mozilla.com) 2011-09-23 10:05:59 PDT
Created attachment 562072 [details] [diff] [review]
Test fixes

These are the purely mechanical test fixes.
Comment 20 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-23 10:08:32 PDT
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 neil@parkwaycc.co.uk 2011-09-27 08:10:00 PDT
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 [:khuey] (khuey@mozilla.com) 2011-09-27 08:11:36 PDT
(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.
Comment 23 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-27 08:12:27 PDT
(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 ...
Comment 25 Kent James (:rkent) 2011-09-29 11:42:27 PDT
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] 2011-09-29 11:43:26 PDT
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 [:khuey] (khuey@mozilla.com) 2011-09-29 11:54:07 PDT
(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 [:khuey] (khuey@mozilla.com) 2011-09-29 12:21:08 PDT
I've backed this out for the moment until we sort this out.

https://hg.mozilla.org/mozilla-central/rev/0eb7a5b286bb
Comment 29 christian 2011-09-29 18:36:05 PDT
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 [:bz] (Out June 25-July 6) 2011-09-29 22:21:28 PDT
> 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 [:bz] (Out June 25-July 6) 2011-09-29 22:25:11 PDT
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] 2011-09-30 05:45:28 PDT
(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 [:bz] (Out June 25-July 6) 2011-09-30 08:03:09 PDT
> 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] 2011-09-30 10:04:14 PDT
(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 [:bz] (Out June 25-July 6) 2011-09-30 10:38:02 PDT
> 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 [:khuey] (khuey@mozilla.com) 2011-09-30 14:50:48 PDT
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] (Out June 27-July 5) 2011-09-30 15:38:39 PDT
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 [:bz] (Out June 25-July 6) 2011-09-30 17:30:37 PDT
Olli, can we remove the nsIScriptContext dependency of nsJSEventListener?  Or come up with an nsIScriptContext in component scope?
Comment 39 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-10-01 01:38:46 PDT
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 [:mao] 2011-10-02 16:44:20 PDT
(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 [:bz] (Out June 25-July 6) 2011-10-02 19:43:22 PDT
> What about Web Workers?

I believe things should continue to work fine there, but worth double-checking, yes.  ;)
Comment 42 Jorge Villalobos [:jorgev] 2011-11-17 13:54:10 PST
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 [:khuey] (khuey@mozilla.com) 2012-08-28 22:07:34 PDT
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.
Comment 44 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-08-29 00:36:02 PDT
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;
Comment 45 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-08-29 15:20:06 PDT
Created attachment 656638 [details] [diff] [review]
Part 1: Explicitly keep track of which event listener holds the event handler
Comment 46 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-08-29 15:21:45 PDT
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.
Comment 47 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-08-30 14:51:42 PDT
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] (high review load, please consider other reviewers) 2012-08-30 15:52:17 PDT
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 [:khuey] (khuey@mozilla.com) 2012-08-30 19:56:41 PDT
(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 [:bz] (Out June 25-July 6) 2012-08-30 20:20:20 PDT
Comment on attachment 656639 [details] [diff] [review]
Part 2: Move event handlers into the event listener manager

r=me on the nsXPConnect change.
Comment 52 :Ms2ger 2012-08-31 01:36:17 PDT
*** Bug 713598 has been marked as a duplicate of this bug. ***
Comment 53 :Ms2ger 2012-08-31 01:37:41 PDT
*** Bug 739044 has been marked as a duplicate of this bug. ***
Comment 55 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-08-31 07:46:25 PDT
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] (Out June 27-July 5) 2012-08-31 07:56:08 PDT
> 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 [:khuey] (khuey@mozilla.com) 2012-08-31 08:04:56 PDT
(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 59 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-12-29 00:01:21 PST
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 [:bz] (Out June 25-July 6) 2012-12-29 00:09:48 PST
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] 2012-12-29 00:24:08 PST
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 [:bz] (Out June 25-July 6) 2012-12-29 00:47:36 PST
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] 2012-12-29 00:56:31 PST
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 [:kohei] 2012-12-29 01:44:43 PST
(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 [:bz] (Out June 25-July 6) 2012-12-29 09:14:23 PST
> 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] 2013-01-05 16:05:34 PST
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 [:kohei] 2013-01-06 02:06:16 PST
The Add-ons Blog post has been updated to point the MDN article.

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