Closed Bug 1180798 Opened 4 years ago Closed 4 years ago

nsIListenerChangeListener: track types of event listener changes

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: lsocks, Assigned: lsocks)

References

Details

Attachments

(3 files, 7 obsolete files)

12.30 KB, patch
Details | Diff | Splinter Review
12.73 KB, patch
Details | Diff | Splinter Review
15.64 KB, patch
smaug
: review+
Details | Diff | Splinter Review
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.
I removed mPendingListenerChangesSet since the same EventTarget can have multiple different event listener changes.
Attachment #8630084 - Flags: review?(bugs)
I would have expected that we collect per EventTarget which all event types have got new listeners or listeners removed.
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
}
Attachment #8630084 - Flags: review?(bugs) → review-
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.
Attachment #8630639 - Flags: feedback?(bugs) → feedback+
The only thing I see is bug 1157469 and that patch hasn't landed yet.
Attachment #8630639 - Attachment is obsolete: true
Attachment #8630686 - Flags: review?(bugs)
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.
Attachment #8630686 - Flags: review?(bugs) → review-
Attached patch patch with test changes (obsolete) — Splinter Review
This test change is causing the following crash: https://pastebin.mozilla.org/8838996
Flags: needinfo?(bugs)
This is the right patch.
Attachment #8631236 - Attachment is obsolete: true
(looks like that patch file has Windows line endings. Please use unix line endings in the future)
link to an interesting stack https://pastebin.mozilla.org/8838974
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
Flags: needinfo?(bugs) → needinfo?(bobbyholley)
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.
Attachment #8631322 - Flags: feedback?(lorien)
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
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.
Flags: needinfo?(bobbyholley)
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).
Attachment #8631322 - Flags: feedback?(lorien)
gabor, you know about sandboxes. Any comments on this one?
Flags: needinfo?(gkrizsanits)
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)
Flags: needinfo?(gkrizsanits)
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.
Flags: needinfo?(gkrizsanits)
(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?
Flags: needinfo?(gkrizsanits)
The test the patch touches.
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
Sounds like we might need Bobby to take (another) look? (see prev comment)
Flags: needinfo?(bobbyholley)
Depends on: 1184382
Flags: needinfo?(bobbyholley)
Assignee: nobody → lorien
Attachment #8630686 - Attachment is obsolete: true
Attachment #8635332 - Flags: review?(bugs)
Attachment #8635332 - Attachment is obsolete: true
Attachment #8635332 - Flags: review?(bugs)
Attachment #8635333 - Flags: review?(bugs)
Fixed a comment in test.
Attachment #8635333 - Attachment is obsolete: true
Attachment #8635333 - Flags: review?(bugs)
Attachment #8635341 - Flags: review?(bugs)
Oops. Sorry about Bugzilla spam, I attached the wrong thing.
Attachment #8635341 - Attachment is obsolete: true
Attachment #8635341 - Flags: review?(bugs)
Attachment #8635343 - Flags: review?(bugs)
Attachment #8635343 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/19dac33769f6
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.