Closed Bug 1157469 Opened 9 years ago Closed 8 years ago

Event listeners bubble doesn't get updated when a listener is added after it's loaded in the markup view

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: bgrins, Assigned: jdescottes)

References

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(3 files, 3 obsolete files)

* Open the inspector on any page and select an element that doesn't yet have any event listeners.
* Open console and type: `$0.addEventListener("click", () => {});`
* Flip back to inspector

Expected:

There is an event bubble next to the node

Actual:

There is no event bubble.  If you close and reopen the inspector it will be there
Attached patch walker-event-listeners-WIP.patch (obsolete) — Splinter Review
Uses the addListenerChangeListener from Bug 524674 to send over mutations when events change for a known target
The debugger events pane will need this, too.
Priority: -- → P2
Whiteboard: [devedition-40]
This should be easy once Bug 524674 lands
Whiteboard: [devedition-40] → [devedition-40][difficulty=easy]
(In reply to Panos Astithas [:past] from comment #2)
> The debugger events pane will need this, too.

It will get updated already in between steps in the debugger, but it won't get updated immediately if the listener was added / removed in the REPL while paused.  That is a smaller usecase, but it will be possible with this API so we may as well support it.
My main concern was the case where the events pane is visible, but the page is running normally (i.e. no debugging currently happens). Will the event list be updated when new event listeners are added?
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
Inspector actor now uses EventListenerService:addListenerChangeListener to propagate
event listener changes as mutations with type 'events'.

markupview will now trigger an update when receiving a mutation of type 'event', and
the event bubble display will be updated

Review commit: https://reviewboard.mozilla.org/r/29739/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29739/
Attachment #8704776 - Flags: review?(bgrinstead)
Assignee: nobody → jdescottes
Comment on attachment 8704776 [details]
MozReview Request: Bug 1157469 - use EventListenerService to update markupview event bubbles;r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29739/diff/1-2/
Comment on attachment 8704776 [details]
MozReview Request: Bug 1157469 - use EventListenerService to update markupview event bubbles;r=bgrins

https://reviewboard.mozilla.org/r/29739/#review26585

LGTM

::: devtools/client/markupview/test/helper_events_test_runner.js:29
(Diff revision 2)
> - *        Selector pointing at the node to be inspected
> + *        node description object

Can you expand on these docs with the possible keys for this object?

::: devtools/server/actors/inspector.js:1360
(Diff revision 2)
> +   * @param Array array

This isn't an actual Array, looks like an nsISimpleEnumerator.  Param name could be updated also to maybe just `changes`

::: devtools/server/actors/inspector.js:1445
(Diff revision 2)
> +      eventListenerService.removeListenerChangeListener(this._onEventListenerChange);

Nit: 80 chars

::: devtools/server/tests/mochitest/test_inspector-mutations-events.html:70
(Diff revision 2)
> +    is(mutations[0].hasEventListeners, true, "mutation target should have event listeners");

May as well check that this value is correct on the front also

::: devtools/server/tests/mochitest/test_inspector-mutations-events.html:105
(Diff revision 2)
> +    is(mutations[0].hasEventListeners, true, "mutation target should have event listeners");

We could also check that the mutation target is eventFront1.actorID (and eventFront2 for the next one).
Attachment #8704776 - Flags: review?(bgrinstead) → review+
Comment on attachment 8704776 [details]
MozReview Request: Bug 1157469 - use EventListenerService to update markupview event bubbles;r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29739/diff/2-3/
this failed to apply:

applying 78bc70b54a7f
unable to find 'devtools/client/markupview/markup-view.js' for patching
3 out of 3 hunks FAILED -- saving rejects to file devtools/client/markupview/markup-view.js.rej
unable to find 'devtools/client/markupview/test/browser_markupview_events.js' for patching
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/markupview/test/browser_markupview_events.js.rej
unable to find 'devtools/client/markupview/test/doc_markup_events.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/markupview/test/doc_markup_events.html.rej
unable to find 'devtools/client/markupview/test/helper_events_test_runner.js' for patching
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/markupview/test/helper_events_test_runner.js.rej
Flags: needinfo?(jdescottes)
Failed to apply on top of https://hg.mozilla.org/integration/fx-team/rev/9c6a36040f06 (Bug 1237335).
Just rebased my changes but 9c6a36040f06 was backed out in the meantime.

New try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=b546c5c658ad
Flags: needinfo?(jdescottes)
Comment on attachment 8704776 [details]
MozReview Request: Bug 1157469 - use EventListenerService to update markupview event bubbles;r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29739/diff/3-4/
https://hg.mozilla.org/mozilla-central/rev/d933300edeed
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
smaug: we started using EventListenerService:addListenerChangeListener and there are intermittent failures which seem linked to it.

15:22:38  WARNING -  PROCESS-CRASH | devtools/client/inspector/test/browser_inspector_search-01.js | application crashed [@ xpc::IsInContentXBLScope(JSObject*)]

[... later in the stacktrace]

15:22:38     INFO -  29  XUL!mozilla::EventListenerService::NotifyPendingChanges() [EventListenerService.cpp:83469fb2d102 : 399 + 0xc]
15:22:38     INFO -      rbp = 0x00007fff5fbfce10   rsp = 0x00007fff5fbfcdd0
15:22:38     INFO -      rip = 0x0000000102a6dedf
15:22:38     INFO -      Found by: previous frame's frame pointer

(full stacktrace is at http://archive.mozilla.org/pub/firefox/tinderbox-builds/fx-team-macosx64/1452208549/fx-team_snowleopard_test-mochitest-devtools-chrome-7-bm108-tests1-macosx-build166.txt.gz

relevant part of the stacktrace attached to the bug)

Do you have any idea what might be wrong here ?
Flags: needinfo?(bugs)
@KWierso : feel free to back this out if it occurs too frequently
Flags: needinfo?(wkocher)
@philor: this bug is most likely the root cause for Bug 1237797. Could you back it out while we are investigating the issue? Sorry about that.
Flags: needinfo?(wkocher) → needinfo?(philringnalda)
Backed out in https://hg.mozilla.org/mozilla-central/rev/d4213241bb79 and merged around.
Status: RESOLVED → REOPENED
Flags: needinfo?(philringnalda)
Resolution: FIXED → ---
Target Milestone: Firefox 46 → ---
Blocks: 1237797
Blocks: 1237883
Attached patch isxblscope_crash.diff (obsolete) — Splinter Review
Looks like a null pointer crash in webidl bindings.
WrapNativeParent calls WrapNativeParentHelper<T>::Wrap and that seems to return null, which is then passed to IsInContentXBLScope.
As far as I see, this is a regression from bug 825392.
http://hg.mozilla.org/mozilla-central/annotate/1424cdfc075d/dom/bindings/BindingUtils.h#l1601 assumes Wrap never fails.
I think we should just change if (!useXBLScope) { few lines above to
if (!useXBLScope || !parent) {
Generated code does have null check for the return value of WrapNativeParent.

Based on the stack trace, the patch should fix the crash.
Whether or not there are other issue is different thing. Something is accessing a xul element rather late.
Does something perhaps forget to remove event listener change listener?



Julian, could you try the patch?
Flags: needinfo?(bugs) → needinfo?(jdescottes)
Attachment #8705696 - Flags: review?(bobbyholley)
Thanks! We cannot reproduce the issue locally, so here is a try push with the patch applied : https://treeherder.mozilla.org/#/jobs?repo=try&revision=b66baa5324ae
Flags: needinfo?(jdescottes)
The try push failed on a similar issue, this time in nsINode::WrapObject(JSContext*, JS::Handle<JSObject*>) which also relies on Wrap.

12:04:24     INFO -  Thread 0 (crashed)
12:04:24     INFO -   0  libxul.so!xpc::IsInContentXBLScope(JSObject*) [jsfriendapi.h:b66baa5324ae : 674 + 0x0]
12:04:24     INFO -      rax = 0x00007ff507932801   rdx = 0x00007fffbf0c45a0
12:04:24     INFO -      rcx = 0x0000000000000000   rbx = 0x0000000000000001
12:04:24     INFO -      rsi = 0x0000000000200020   rdi = 0x0000000000000000
12:04:24     INFO -      rbp = 0x00007fffbf0c4540   rsp = 0x00007fffbf0c44e0
12:04:24     INFO -       r8 = 0x0000000000000001    r9 = 0x00007ff512da2ad0
12:04:24     INFO -      r10 = 0x00007fffbf0c42c8   r11 = 0x00007ff512da2ad0
12:04:24     INFO -      r12 = 0x00007ff4ce1f2ee0   r13 = 0x00007fffbf0c4598
12:04:24     INFO -      r14 = 0x0000000000000000   r15 = 0x00007ff4f3bfe8b0
12:04:24     INFO -      rip = 0x00007ff50e02c725
12:04:24     INFO -      Found by: given as instruction pointer in context
12:04:24     INFO -   1  libxul.so!nsINode::WrapObject(JSContext*, JS::Handle<JSObject*>) [nsINode.cpp:b66baa5324ae : 2782 + 0x9]
12:04:24     INFO -      rbx = 0x0000000000000001   rbp = 0x00007fffbf0c4540
12:04:24     INFO -      rsp = 0x00007fffbf0c44f0   r12 = 0x00007ff4ce1f2ee0
12:04:24     INFO -      r13 = 0x00007fffbf0c4598   r14 = 0x0000000000000000
12:04:24     INFO -      r15 = 0x00007ff4f3bfe8b0   rip = 0x00007ff50e54e6c9
12:04:24     INFO -      Found by: call frame info
12:04:24     INFO -   2  libxul.so!mozilla::dom::Element::WrapObject(JSContext*, JS::Handle<JSObject*>) [Element.cpp:b66baa5324ae : 432 + 0x16]
12:04:24     INFO -      rbx = 0x00007ff4ce1f2ee0   rbp = 0x00007fffbf0c4620
12:04:24     INFO -      rsp = 0x00007fffbf0c4550   r12 = 0x00007fffbf0c490c
12:04:24     INFO -      r13 = 0x0000000000000000   r14 = 0x0000000000000000
12:04:24     INFO -      r15 = 0x00007ff4f3bfe8b0   rip = 0x00007ff50e488ae1
12:04:24     INFO -      Found by: call frame info
12:04:24     INFO -   3  libxul.so!XPCConvert::NativeInterface2JSObject(JS::MutableHandle<JS::Value>, nsIXPConnectJSObjectHolder**, xpcObjectHelper&, nsID const*, XPCNativeInterface**, bool, nsresult*) [XPCConvert.cpp:b66baa5324ae : 785 + 0x30]
12:04:24     INFO -      rbx = 0x0000000000000000   rbp = 0x00007fffbf0c47a0
12:04:24     INFO -      rsp = 0x00007fffbf0c4630   r12 = 0x00007fffbf0c490c
12:04:24     INFO -      r13 = 0x00007ff4ce1f2ee8   r14 = 0x0000000000000000
12:04:24     INFO -      r15 = 0x00007ff4f3bfe8b0   rip = 0x00007ff50e04d635
12:04:24     INFO -      Found by: call frame info
12:04:24     INFO -   4  libxul.so!XPCConvert::NativeData2JS(JS::MutableHandle<JS::Value>, void const*, nsXPTType const&, nsID const*, nsresult*) [XPCConvert.cpp:b66baa5324ae : 342 + 0x25]
12:04:24     INFO -      rbx = 0x00007ff4ce1f2ee0   rbp = 0x00007fffbf0c48c0
12:04:24     INFO -      rsp = 0x00007fffbf0c47b0   r12 = 0x00007fffbf0c490c
12:04:24     INFO -      r13 = 0x00007fffbf0c4920   r14 = 0x00007fffbf0c4907
12:04:24     INFO -      r15 = 0x0000000000000000   rip = 0x00007ff50e04db73
12:04:24     INFO -      Found by: call frame info

(full log at http://archive.mozilla.org/pub/firefox/try-builds/jdescottes@mozilla.com-b66baa5324ae2fe2b40e73153908b2ff557f358b/try-linux64-debug/try_ubuntu64_vm-debug_test-mochitest-devtools-chrome-5-bm122-tests1-linux64-build225.txt.gz)

I will check if the event listener change listener is properly removed.
smaug: new stack trace in Comment 24 if you want to have a look
Flags: needinfo?(bugs)
Attached patch v2 for the crash (obsolete) — Splinter Review
Julian, want to try this one then. Just added a null check !obj
Attachment #8705696 - Attachment is obsolete: true
Attachment #8705696 - Flags: review?(bobbyholley)
Flags: needinfo?(bugs) → needinfo?(jdescottes)
Attachment #8705861 - Flags: review?(bobbyholley)
These crashes do hint that event listener change listener is called easily with event targets which are from a scope which is already gone. Which is expected given that the callback is async.
@smaug : thanks! new try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=da6aba6da689
Flags: needinfo?(jdescottes)
Some builds failed on the previous try push. 

Triggered another one : https://treeherder.mozilla.org/#/jobs?repo=try&revision=c03e1960ae4d , no errors.
Relaunched another try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=49d14f3b869e
Smaug: your last patch fixes the issue. How do we proceed to land this?
Flags: needinfo?(bugs)
the patch needs a review. Once it has r+, feel free to land with other patches.
Flags: needinfo?(bugs)
Comment on attachment 8705861 [details] [diff] [review]
v2 for the crash

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

::: dom/base/nsINode.cpp
@@ +2780,5 @@
>  
>    JS::Rooted<JSObject*> obj(aCx, WrapNode(aCx, aGivenProto));
>    MOZ_ASSERT_IF(ChromeOnlyAccess(),
> +                !obj || xpc::IsInContentXBLScope(obj) ||
> +                !xpc::UseContentXBLScope(js::GetObjectCompartment(obj)));

Nit: I think it would be clearer to include the null check in the "IF" part of the assert. So:

MOZ_ASSERT_IF(obj && ChromeOnlyAccess(),
              xpc::IsInContentXBLScope(obj) || xpc::UseContentXBLScope(js::GetObjectCompartment(obj)));

::: dom/bindings/BindingUtils.h
@@ +1593,5 @@
>      return JS::CurrentGlobalOrNull(cx);
>    }
>  
>    JSObject* parent = WrapNativeParentHelper<T>::Wrap(cx, p, cache);
> +  if (!useXBLScope || !parent) {

Nit: I think it would be clearer to switch the order of these two checks. So:

if (!parent || !useXBLScope) {
Attachment #8705861 - Flags: review?(bobbyholley) → review+
Attachment #8597419 - Attachment is obsolete: true
Comment on attachment 8706500 [details] [diff] [review]
+comments

Carrying over the R+ from smaug's last patch
Attachment #8706500 - Flags: review+
Attachment #8705861 - Attachment is obsolete: true
Comment on attachment 8704776 [details]
MozReview Request: Bug 1157469 - use EventListenerService to update markupview event bubbles;r=bgrins

https://reviewboard.mozilla.org/r/29739/#review27229
Attachment #8704776 - Flags: review+
Last try push was successful, adding checkin-needed for both patches
Keywords: checkin-needed
this failed to apply:

applying 4451dc137869
unable to find 'devtools/client/markupview/markup-view.js' for patching
3 out of 3 hunks FAILED -- saving rejects to file devtools/client/markupview/markup-view.js.rej
unable to find 'devtools/client/markupview/test/browser_markupview_events.js' for patching
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/markupview/test/browser_markupview_events.js.rej
unable to find 'devtools/client/markupview/test/doc_markup_events.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/markupview/test/doc_markup_events.html.rej
unable to find 'devtools/client/markupview/test/helper_events_test_runner.js' for patching
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/markupview/test/helper_events_test_runner.js.rej
file devtools/server/tests/mochitest/test_inspector-mutations-events.html already exists
1 out of 1 hunks FAILED -- saving rejects to file devtools/server/tests/mochitest/test_inspector-mutations-events.html.rej
patch failed to apply
abort: fix up the merge and run hg transplant --continue

could you take a look, thanks!
Flags: needinfo?(jdescottes)
Comment on attachment 8704776 [details]
MozReview Request: Bug 1157469 - use EventListenerService to update markupview event bubbles;r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29739/diff/4-5/
I reapplied the backed-out changeset locally using `hg graft -f d933300edeed`, which merged without conflicts. This is what I did yesterday as well for my try push, so I don't think a new try push is needed. I uploaded this new revision to mozreview.

Could you try with the latest patch uploaded to reviewboard ? (131c9e4a65750af75a9fef8d3e59b755a18bf296)
Flags: needinfo?(jdescottes) → needinfo?(cbook)
Flags: needinfo?(cbook)
https://hg.mozilla.org/mozilla-central/rev/3791e857a33f
https://hg.mozilla.org/mozilla-central/rev/dd3a10e8fcce
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
Test Successful.

Component: 
Name 			Firefox
Version 		46.0b4
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64


Expected Results: 
There is an event bubble next to the node

Actual Results: 
As expected
Depends on: 1269834
Depends on: 1327727
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.