Lorien Hu (:lsocks) Assignee
	Description • 9 years ago

nsIListenerChangeListener is currently only notified of the EventTargets which have had listeners added or removed. It would be useful to also pass the type of event listener that's been changed.

	Lorien Hu (:lsocks) Assignee
	Comment 1 • 9 years ago

I removed mPendingListenerChangesSet since the same EventTarget can have multiple different event listener changes.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 2 • 9 years ago

I would have expected that we collect per EventTarget which all event types have got new listeners or listeners removed.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 3 • 9 years ago

So, listenersChanged would not take nsIArray, but
some new interface, say EventTargetWithChangedListeners.


interface EventTargetWithChangedListeners
{
  readonly attribute nsIDOMEventTarget target;
  readonly attribute nsIArray changedListenerNames; // Array of nsIAtoms
}

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 9 years ago

Comment on attachment 8630639 [details] [diff] [review]
Store event listener changes in EventListenerChange and pass to EventListenerChangeListeners.

>+class EventListenerChange final : public nsIEventListenerChange
>+{
>+public:
>+  EventListenerChange(dom::EventTarget* aTarget);
>+
>+  void AddChangedListenerName(nsIAtom* aEventName);
>+
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIEVENTLISTENERCHANGE
>+
>+protected:
>+  virtual ~EventListenerChange();
>+  dom::EventTarget* mTarget;
Never ever raw pointers in classes which are allocated from heap - if possible.
So, this should be nsCOMPtr


Yeah, this looks good. I think, but not sure, that you need to change some devtools code using the old API.

	Lorien Hu (:lsocks) Assignee
	Comment 6 • 9 years ago

The only thing I see is bug 1157469 and that patch hasn't landed yet.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 7 • 9 years ago

Comment on attachment 8630686 [details] [diff] [review]
Store event listener changes in EventListenerChange and pass to EventListenerChangeListeners.


>+/**
>+ * Contains an event target along with a list of changed event listeners.

Could you say also that the list is an array of nsIAtom objects in form
oneventname


And test_bug524674.xul sure will break with this.
Update that and add checks for right event types there.

	Lorien Hu (:lsocks) Assignee
	Comment 8 • 9 years ago

This test change is causing the following crash: https://pastebin.mozilla.org/8838996

	Lorien Hu (:lsocks) Assignee
	Comment 9 • 9 years ago

This is the right patch.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 10 • 9 years ago

(looks like that patch file has Windows line endings. Please use unix line endings in the future)

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 11 • 9 years ago

link to an interesting stack https://pastebin.mozilla.org/8838974

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 12 • 9 years ago

As far as I see the crash is a regression from bug 825392.
There used to be a null check for obj, but it was removed.
bholley, was that on purpose?

See also comment 11

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 13 • 9 years ago

Lorien, I added a null check to nsINode.cpp, removed a printf and changed where we have catch(ex) {} in the test.
Could you test this and if this is looks ok to me, ask review.
(this is after all mostly your code) 


I'll ask bholley to check the nsINode.cpp change.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 14 • 9 years ago

bholley, some relevant warnings when running the test with
https://bugzilla.mozilla.org/attachment.cgi?id=8631239

[35509] WARNING: NS_ENSURE_SUCCESS(rv, nullptr) failed with result 0x80070057: file /Users/lhu/Mozilla/mozilla-central/js/xpconnect/src/XPCWrappedNativeScope.cpp, line 290
[35509] WARNING: NS_ENSURE_TRUE(scope) failed: file /Users/lhu/Mozilla/mozilla-central/js/xpconnect/src/XPCWrappedNativeScope.cpp, line 318
[35509] WARNING: NS_ENSURE_TRUE(xblScope) failed: file ../../dist/include/mozilla/dom/BindingUtils.h, line 1567

	Bobby Holley (:bholley)
	Comment 15 • 9 years ago

The null check is fine, though I'd move it into the condition of the MOZ_ASSERT_IF.

I'm not sure why we'd be failing to create a sandbox - seems worth investigating.

	Lorien Hu (:lsocks) Assignee
	Comment 16 • 9 years ago

I'm getting a leak with that last patch:

== BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, default process 33678

     |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
     |                                      | Per-Inst   Leaked|   Total      Rem|
   0 |TOTAL                                 |       31       80| 1126332        5|
 424 |ProtoAndIfaceCache                    |       16       80|     285        5|

It looks like this is related to that crash, since it seems to leak when we fail to create a sandbox. At least this doesn't occur when I change the test to examine only EventTargets that are a part of our test (and thus don't trigger the failures).

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 17 • 9 years ago

gabor, you know about sandboxes. Any comments on this one?

	Gabor Krizsanits (INACTIVE)
	Comment 18 • 9 years ago

As Bobby said, it's very odd that the sandbox creation fails, can we debug it why that happens? (Btw. it's not clear which test produces the leak in Comment 16 and Comment 11 is outdated)

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 19 • 9 years ago

Well, I was kind of hoping that you gabor or bholley would take a look at the sandbox failure, since you are familiar with the relevant code and probably even know what the code is supposed to do ;)

But if not, I guess I'll take a look then.

	Gabor Krizsanits (INACTIVE)
	Comment 20 • 9 years ago

(In reply to Olli Pettay [:smaug] from comment #19)
> Well, I was kind of hoping that you gabor or bholley would take a look at
> the sandbox failure

I can do that, just I don't know how to get there, which test should I run for it with the patch?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 21 • 9 years ago

The test the patch touches.

	Gabor Krizsanits (INACTIVE)
	Comment 22 • 9 years ago

Checked unwrap fails. The proto is from nsNestedAboutURI the sandbox is created with expanded principal.
So it seems like we want to create the XBL scope too early? Don't know this part that well tbh...

stack: https://pastebin.mozilla.org/8839350

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 23 • 9 years ago

Sounds like we might need Bobby to take (another) look? (see prev comment)

	Bobby Holley (:bholley)
	Comment 24 • 9 years ago

I filed bug 1184382.

	Lorien Hu (:lsocks) Assignee
	Comment 27 • 9 years ago

Fixed a comment in test.

	Lorien Hu (:lsocks) Assignee
	Comment 28 • 9 years ago

Oops. Sorry about Bugzilla spam, I attached the wrong thing.

	Pulsebot
	Comment 29 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/19dac33769f6

	Carsten Book [:Tomcat]
	Comment 30 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/19dac33769f6