The default bug view has changed. See this FAQ.

don't rely on slots to hold event handlers, IDL-ify event handlers

RESOLVED FIXED in mozilla9

Status

()

Core
DOM: Events
P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: gal, Assigned: bz)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs, {dev-doc-complete})

Trunk
mozilla9
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 6 obsolete attachments)

379 bytes, text/html
Details
12.68 KB, patch
smaug
: review+
peterv
: review+
Details | Diff | Splinter Review
8.52 KB, patch
smaug
: review+
Details | Diff | Splinter Review
11.21 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.54 KB, patch
smaug
: review+
Details | Diff | Splinter Review
96.84 KB, patch
mrbkap
: review+
smaug
: review+
peterv
: review+
Details | Diff | Splinter Review
51.42 KB, patch
Details | Diff | Splinter Review
12.80 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.32 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Reporter)

Comment 1

6 years ago
Created attachment 534803 [details] [diff] [review]
patch
Assignee: nobody → gal
(Reporter)

Updated

6 years ago
Summary: don't rely on slots to hold event handlers → don't rely on slots to hold event handlers, IDL-ify event handlers
Would this help with bug 414853?
Yes.
That seems to be passing PR_FALSE as a jsval to SetJSEventListener?
Also, how is this managing to remove BindCompiledEventHandler?
I was thinking to do this in a bit different way, but this is probably better.
nsDOMEventTargetHelper needs to be converted to use this for onfoo, and just
disallow C++ onfoo listener implementations. 

Also, the patch is for nsIDOMHTMLElement only. SVG, XUL etc elements, documents and window need to be changed too.
(Reporter)

Comment 7

6 years ago
Created attachment 535026 [details] [diff] [review]
patch

changes for window, body and frameset. svg still missing.
Attachment #534803 - Attachment is obsolete: true
(Reporter)

Comment 8

6 years ago
BindCompiledEventHandler: the handler is now passed around as a jsval and is given to SetJSEventListener direct, it no longer looks it up on the object when the event is triggered so no need to bind there.
(Reporter)

Comment 9

6 years ago
Created attachment 535080 [details] [diff] [review]
patch

getting closer
Attachment #535026 - Attachment is obsolete: true
Comment on attachment 535080 [details] [diff] [review]
patch

So atm we support those onfoo listeners on all elements, not just HTML/SVG/XUL.
Yeah; it's not clear whether this is useful.  Does any other browser do that?
(Reporter)

Comment 12

6 years ago
Created attachment 535420 [details] [diff] [review]
patch

Complete patch (XULElements might not bind the event handler properly, have to double check that). Patch compiles, but crashes horribly on startup.
Attachment #535080 - Attachment is obsolete: true
(Reporter)

Comment 13

6 years ago
Created attachment 535635 [details] [diff] [review]
patch
Attachment #535420 - Attachment is obsolete: true
(Reporter)

Comment 14

6 years ago
XULElements register the event handler with the event manager now. Patch now crashes because of a NULL event manager. We probably have to instantiate that lazily in that spot. The other thing that is known broken is that XULElement and potentially SVG need their IDLs updated to add the event handler attributes (we only did HTML so far).
(Reporter)

Updated

6 years ago
Assignee: gal → bzbarsky
(Reporter)

Updated

6 years ago
Blocks: 580070
Or should we just add the onfoo attributes to nsIDOMElement.idl
See comment 11?
Created attachment 535686 [details]
testcase

Seems like Opera and Chrome and M-C all support onfoo with Element.
No need for HTMLElement.
Attachment #535686 - Attachment is patch: false
Attachment #535686 - Attachment mime type: text/plain → text/html
(Reporter)

Comment 18

6 years ago
Should we propose to fix the spec?
Which spec?
HTML5, probably, which currently requires that these properties appear on HTMLElement.prototype.  And Web DOM Core, which should require them on Element....
http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Element.idl
Webkit seems to have onfoo in the Element.idl, not in HTMLElement.idl
Yeah, I'm going to put this on Element.  And we just need to get the specs fixed.
Works for me, care to email public-webapps?
Priority: -- → P1
Blocks: 657994
Depends on: 671453
Depends on: 675267
Depends on: 675405
Blocks: 676750
Depends on: 626563
Created attachment 551219 [details] [diff] [review]
Part 1.  Add an interface to documents to hold the touch event properties.
Attachment #551219 - Flags: review?(peterv)
Attachment #551219 - Flags: review?(Olli.Pettay)
Attachment #535635 - Attachment is obsolete: true
Created attachment 551220 [details] [diff] [review]
part 2.  Add nsITouchEventReceiver for windows.
Attachment #551220 - Flags: review?(Olli.Pettay)
Created attachment 551221 [details] [diff] [review]
part 3.  Add nsITouchEventReceiver for elements.
Attachment #551221 - Flags: review?(Olli.Pettay)
Created attachment 551223 [details] [diff] [review]
part 4.  Change some tests that depend on the old behavior where inline event handlers hang directly off the JSObject to not depend on it.
Attachment #551223 - Flags: review?(Olli.Pettay)
Created attachment 551224 [details] [diff] [review]
part 5a.  Add the new idl, with stubbed-out impls for now.

This patch can't actually land on its own, because it gives somewhat broken behavior, but I wanted to break it out from the rest of the changes for ease of review.  The reason we need the separate interface for the common attributes is that otherwise some element sub-interfaces end up with too many vtable entries for xpconnect to represent in its stubs, but those interfaces are implemented in XBL so need to be stubbed.
Attachment #551224 - Flags: review?(jonas)
Attachment #551224 - Flags: review?(Olli.Pettay)
Created attachment 551226 [details] [diff] [review]
Part 5b: main changes

A few other notes:
1) test_bug583225.html may no longer be testing what it wants to test; I'm not sure how to make it test that.
2) I'd appreciate a close look at my claims about cycle collection stuff in SetHandler.
3) I'd likewise appreciate a close look at the way SetJSEventListenerToJsval gets an
   nsIScriptContext.
4) I didn't attempt to match the spec's terminology for now; we may want to do a followup for that.
Attachment #551226 - Flags: review?(mrbkap)
Attachment #551226 - Flags: review?(Olli.Pettay)
Oh, and my apologies for the size of that last patch.  I couldn't figure out a good way to do it incrementally while keeping things compiling for each changeset without some serious gymnastics that seemed uncalled for.  The good news is that so much of it is code removal... ;)

Also, modulo what looks like permaorange on bug 626563, by the way, this should be passing tests.  I'll figure out that permaorange before landing.  There will also be a part 6 with additional tests, probably on Monday.  Notes to self on that:

* Test "on* in foo".
* Test that onfoo properties are correctly updated when onfoo attributes
  are set.
* Test that forwarding to the window works correctly.
* Test |this| and scope handling stuff.
Whiteboard: [need review]
Attachment #551224 - Flags: review?(jonas) → review+
Attachment #551219 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 551220 [details] [diff] [review]
part 2.  Add nsITouchEventReceiver for windows.


>+  // XXXbz should ChromeWindow really not have touch, indexeddb,
>+  // performance apis?
It certainly should have touch, and performance should be ok to.
Could you please add those, or file a followup to do so.

Please ask bent or sicking if indexeddb is supported in chrome.

> 
>+  // XXXbz should ModalContentWindow really not have touch, indexeddb,
>+  // performance apis?
It certainly should have all those. Fix or file a followup.
There should be some macro for all the common window interfaces.
Attachment #551220 - Flags: review?(Olli.Pettay) → review+
Attachment #551221 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 551223 [details] [diff] [review]
part 4.  Change some tests that depend on the old behavior where inline event handlers hang directly off the JSObject to not depend on it.

I hope losing support for function onload() {} doesn't regress too badly.
Attachment #551223 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 551224 [details] [diff] [review]
part 5a.  Add the new idl, with stubbed-out impls for now.


>+           [implicit_jscontext] attribute jsval            onafterprint;
>+           [implicit_jscontext] attribute jsval            onbeforeprint;
>+           [implicit_jscontext] attribute jsval            onbeforeunload;
>+           [implicit_jscontext] attribute jsval            onhashchange;
>+           [implicit_jscontext] attribute jsval            onmessage;
>+           [implicit_jscontext] attribute jsval            onoffline;
>+           [implicit_jscontext] attribute jsval            ononline;
>+           [implicit_jscontext] attribute jsval            onpagehide;
>+           [implicit_jscontext] attribute jsval            onpageshow;
>+           [implicit_jscontext] attribute jsval            onpopstate;
>+  // Not supported yet
>+  // [implicit_jscontext] attribute jsval            onredo;
>+           [implicit_jscontext] attribute jsval            onresize;
>+  // Not supported yet
>+  // [implicit_jscontext] attribute jsval            onstorage;
>+  // Not supported yet
>+  // [implicit_jscontext] attribute jsval            onundo;
>+           [implicit_jscontext] attribute jsval            onunload;
Something strange with indentation.


> interface nsIDOMHTMLFrameSetElement : nsIDOMHTMLElement
...
>+           [implicit_jscontext] attribute jsval            onpopstate;
>+  // Not supported yet
>+  // [implicit_jscontext] attribute jsval            onredo;
>+           [implicit_jscontext] attribute jsval            onresize;
>+  // Not supported yet
>+  // [implicit_jscontext] attribute jsval            onstorage;
>+  // Not supported yet
>+  // [implicit_jscontext] attribute jsval            onundo;
>+           [implicit_jscontext] attribute jsval            onunload;
ditto
Attachment #551224 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 551226 [details] [diff] [review]
Part 5b: main changes


>+#define EVENT(name_, id_, type_, struct_)                                    \
>+  nsresult nsINode::GetOn##name_(JSContext *cx, jsval *vp) {                 \
>+    nsEventListenerManager *elm = GetListenerManager(PR_FALSE);              \
>+    if (elm) {                                                               \
>+      elm->GetJSEventListener(nsGkAtoms::on##name_, vp);                     \
>+    } else {                                                                 \
>+      *vp = JSVAL_NULL;                                                      \
>+    }                                                                        \
>+    return NS_OK;                                                            \
>+  }                                                                          \
>+  nsresult nsINode::SetOn##name_(JSContext *cx, const jsval &v) {            \
>+    nsEventListenerManager *elm = GetListenerManager(PR_TRUE);               \
>+    if (!elm) {                                                              \
>+      return NS_ERROR_OUT_OF_MEMORY;                                         \
>+    }                                                                        \
'new' is infallible


>+                                                                             \
>+    JSObject *obj = GetWrapper();                                            \
>+    if (!obj) {                                                              \
>+      return NS_ERROR_UNEXPECTED;                                            \
>+    }                                                                        \
In which case could this happen? Throwing is quite unfortunate.
Could NS_ENSURE_TRUE(obj, NS_OK); be enough.


> nsEventListenerManager::SetJSEventListener(nsIScriptContext *aContext,
>                                            void *aScopeObject,
>                                            nsIAtom* aName,
>-                                           PRBool aIsString,
>-                                           PRBool aPermitUntrustedEvents)
>+                                           JSObject *aHandler,
>+                                           PRBool aPermitUntrustedEvents,
>+                                           nsListenerStruct **aListenerStruct)
> {
>   nsresult rv = NS_OK;
>   PRUint32 eventType = nsContentUtils::GetEventId(aName);
>   nsListenerStruct* ls = FindJSEventListener(eventType, aName);
> 
>   if (!ls) {
>     // If we didn't find a script listener or no listeners existed
>     // create and add a new one.
>     nsCOMPtr<nsIDOMEventListener> scriptListener;
>     rv = NS_NewJSEventListener(aContext, aScopeObject, mTarget, aName,
>-                               getter_AddRefs(scriptListener));
>+                               aHandler, getter_AddRefs(scriptListener));
>     if (NS_SUCCEEDED(rv)) {
>       AddEventListener(scriptListener, eventType, aName, nsnull,
>                        NS_EVENT_FLAG_BUBBLE | NS_PRIV_EVENT_FLAG_SCRIPT);
> 
>       ls = FindJSEventListener(eventType, aName);
>     }
>+  } else {
>+    ls->GetJSListener()->SetHandler(aHandler);
>   }
This is a behavior we want to change.
We should remove the old listener and add a new one so that
the newly added listener will be at the end of the event listener list.
Please file a followup.


>+    ls->mHandlerIsString = !aHandler;
I hope !aHandler parameter value is documented



>+    if (handlerOwner) {
>+      // Always let the handler owner compile the event
>+      // handler, as it may want to use a special
>+      // context or scope object.
>+      result = handlerOwner->CompileEventHandler(context,
>+                                                 aListenerStruct->mTypeAtom,
>+                                                 *aBody,
>+                                                 url.get(), lineNo,
>+                                                 handler);
>+    }
>+    else {
} else { 


>+nsresult
>+nsEventListenerManager::SetJSEventListenerToJsval(nsIAtom *aEventName,
>+                                                  JSContext *cx,
>+                                                  JSObject* aScope,
>+                                                  const jsval & v)
an extra space before or after &
I'd prefer nsIAtom* ... etc



>+  nsIJSEventListener* GetJSListener() const {
{ should be in the next line

>+    if (!(mFlags & NS_PRIV_EVENT_FLAG_SCRIPT))
>+      return nsnull;
if (expr) {
  stmt;
}
>+    return static_cast<nsIJSEventListener *>(mListener.get());

Or, perhaps in this case expr : val1 : val2;


>   if (id == sOnload_id || id == sOnerror_id) {
>     // Make sure that this node can't go away while waiting for a
>     // network load that could fire an event handler.
>+    // XXXbz won't this fail if the listener is added using
>+    // addEventListener?  On the other hand, even if I comment this
>+    // code out I can't seem to reproduce the bug it was trying to
>+    // fix....
>     nsNodeSH::PreserveWrapper(GetNative(wrapper, obj));
>   }
Please file a followup to sort this out.

>+  NS_IMETHODIMP nsGlobalWindow::SetOn##name_(JSContext *cx,                  \
>+                                             const jsval &v) {               \
>+    nsEventListenerManager *elm = GetListenerManager(PR_TRUE);               \
>+    if (!elm) {                                                              \
>+      return NS_ERROR_OUT_OF_MEMORY;                                         \
>+    }                                                                        \
>+                                                                             \
>+    JSObject *obj = mJSObject;                                               \
>+    if (!obj) {                                                              \
>+      return NS_ERROR_UNEXPECTED;                                            \
>+    }                                                                        \
Same comments here as earlier.


>@@ -113,59 +120,39 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mTarget)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mContext)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> 
> NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(nsJSEventListener)
>   NS_IMPL_CYCLE_COLLECTION_TRACE_MEMBER_CALLBACK(tmp->mContext->GetScriptTypeID(),
>                                                  mScopeObject)
>+  NS_IMPL_CYCLE_COLLECTION_TRACE_MEMBER_CALLBACK(tmp->mContext->GetScriptTypeID(),
>+                                                 mHandler)
> NS_IMPL_CYCLE_COLLECTION_TRACE_END
Peterv should look at this.


Looking good.
Changes to ELM were a bit tricky to read, I hope I didn't missed anything important.
Attachment #551226 - Flags: review?(Olli.Pettay) → review+
> It certainly should have touch, and performance should be ok to.

Filed bug 680903.

> I hope losing support for function onload() {} doesn't regress too badly.

It's not supported in IE or WebKit or Opera, so I hope so too.

> Something strange with indentation.

Will fix.

> 'new' is infallible

But adding an entry to a pldhash is not, and GetListenerManager can return null if that fails.  So I think I do need the null-check for now.

> In which case could this happen?

If someone manually calls this method from C++ on an object that's never been touched from JS is the only way I can think of.  I'm fine with returning NS_OK in that situation if you prefer.  Let me know, please.

> We should remove the old listener and add a new one
> Please file a followup.

Bug 680913.

> I hope !aHandler parameter value is documented

API comments for SetJSEventListener say:

  aHandler may be null to indicate that we should lazily get and compile
  the string for this listener.  

> } else { 

Fixed.

> an extra space before or after &

Removed space after.

> I'd prefer nsIAtom* ... etc

Hmm...  I've been convinced that space before '*' is better in general, and the file is already inconsistent about it (e.g. uses nsIAtom* but for JS types puts a space before the *).  If you feel strongly about it, I'll make the change, I guess.  Let me know, please.

> Or, perhaps in this case expr : val1 : val2;

Done.

> Please file a followup to sort this out.

Bug 680916.

> Peterv should look at this.

OK.

> Changes to ELM were a bit tricky to read

Yeah, I'm really sorry about that...  :(
Blocks: 680903, 680913, 680916
Attachment #551226 - Flags: review?(peterv)
(In reply to Boris Zbarsky (:bz) from comment #35)
> But adding an entry to a pldhash is not, and GetListenerManager can return
> null if that fails.  So I think I do need the null-check for now.
Ah, ok.


> If someone manually calls this method from C++ on an object that's never
> been touched from JS is the only way I can think of.  I'm fine with
> returning NS_OK in that situation if you prefer.  Let me know, please.
I'd prefer NS_OK.



> Hmm...  I've been convinced that space before '*' is better in general, and
> the file is already inconsistent about it (e.g. uses nsIAtom* but for JS
> types puts a space before the *).  If you feel strongly about it, I'll make
> the change, I guess.  Let me know, please.
No need to make the change.
Comment on attachment 551219 [details] [diff] [review]
Part 1.  Add an interface to documents to hold the touch event properties.

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

::: dom/base/nsDOMClassInfo.cpp
@@ +2175,5 @@
> +    /* Compact the interface list */                                          \
> +    size_t count = NS_ARRAY_LENGTH(interface_list);                           \
> +    /* count is the number of array entries, which is one greater than the */ \
> +    /* number of interfaces due to the terminating null */                    \
> +    for (size_t i = 0; i < count-1; ++i) {                                    \

Spaces around -.
Attachment #551219 - Flags: review?(peterv) → review+
> I'd prefer NS_OK.

Done.

> Spaces around -.

Fixed.

Going to post a patch with the tests.  Running those found that when I moved the new properties to a tearoff all the shim gunk for body/frameset stopped working correctly.  I've fixed that by just making the new nsINode methods virtual and overriding them on body and frameset as needed instead of doing the shims and will attach the updated version of part 5a and the interdiff...  Yay tests!
Created attachment 555034 [details] [diff] [review]
Part 5a fixed so we pass tests
Attachment #551224 - Attachment is obsolete: true
Created attachment 555035 [details] [diff] [review]
Interdiff for 5a
Attachment #555035 - Flags: review?(Olli.Pettay)
Created attachment 555036 [details] [diff] [review]
part 6.  Add various tests for the on* properties.
Attachment #555036 - Flags: review?(Olli.Pettay)
Attachment #555035 - Flags: review?(Olli.Pettay) → review+
Attachment #555036 - Flags: review?(Olli.Pettay) → review+
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Comment on attachment 551226 [details] [diff] [review]
Part 5b: main changes

I asked bz on IRC to add a local pointer alias for aBody in CompileEventHandlerInternal.
Attachment #551226 - Flags: review?(mrbkap) → review+
Comment on attachment 551226 [details] [diff] [review]
Part 5b: main changes

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

::: dom/src/events/nsJSEventListener.cpp
@@ +87,5 @@
>    nsContentUtils::HoldScriptObject(aContext->GetScriptTypeID(), this,
>                                     &NS_CYCLE_COLLECTION_NAME(nsJSEventListener),
>                                     aScopeObject, PR_FALSE);
> +  if (aHandler) {
> +    nsContentUtils::HoldScriptObject(aContext->GetScriptTypeID(), this,

Technically this is correct, though it'll be a no-op. Once we rip out all the DOM agnostic code this should just be one call to |HoldJSObjects(this, &NS_CYCLE_COLLECTION_NAME(nsJSEventListener));| and the |DropScriptObjects(...)| calls should be |DropJSObjects(...)|.
Attachment #551226 - Flags: review?(peterv) → review+
http://hg.mozilla.org/mozilla-central/rev/3fbd9eaf9deb
http://hg.mozilla.org/mozilla-central/rev/1f58f9ed470c
http://hg.mozilla.org/mozilla-central/rev/e45c1629b10c
http://hg.mozilla.org/mozilla-central/rev/2b814584bb2a
http://hg.mozilla.org/mozilla-central/rev/233cdba24f03
http://hg.mozilla.org/mozilla-central/rev/a9677078eb73
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Since someone asked, the reason we're doing this is because it simplifies the code, makes us actually match the spec, fixes some longstanding web compat issues, and makes it easier to implement the new DOM bindings for elements.

Updated

6 years ago
Depends on: 682554

Updated

6 years ago
Depends on: 682637

Updated

6 years ago
Depends on: 684671
Blocks: 685844
Blocks: 639720, 660233

Updated

6 years ago
Depends on: 689564

Comment 46

6 years ago
>> I hope losing support for function onload() {} doesn't regress too badly.
> It's not supported in IE or WebKit or Opera, so I hope so too.

This should be mentioned in the docs. I know I was used to that construct when doing quick testing. Are there other developer-visible changes from this?
Keywords: dev-doc-needed
Er, yeah.  Good catch on dev-doc-needed.

There are various developer-visible changes.  "in" now correctly works to test for supported on* attributes.  Setting on* attributes on the relevant prototypes throws, with some exceptions that we'd rather not document because we want to remove them.  The properties now claim to be accessor properties if you ask for their property descriptor (before they were either not present or data properties).  You can now shadow these properties, like any other property on the prototype.  Probably a bunch of others; basically all the ways IDL-defined properties differ from own data properties.
Whiteboard: [need review]
Duplicate of this bug: 697002
Blocks: 517143
I've done some documentation work on this, as much as is likely to happen for now. The key thing is the addition of this article, which has a section that explains these changes:

https://developer.mozilla.org/en/DOM/DOM_event_handlers

This is also linked to from:

https://developer.mozilla.org/en/DOM/document#Event_handlers
https://developer.mozilla.org/en/DOM/window#Event_handlers
https://developer.mozilla.org/en/DOM/element#Event_Handlers

I've also added a link to that from Firefox 9 for developers. We'll add more links to this as needed as we discover places for it in the future, but for now this is documentation complete.
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 763225
Blocks: 414853
You need to log in before you can comment on or make changes to this bug.