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)
DevTools
Inspector
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
Reporter | ||
Comment 1•9 years ago
|
||
Uses the addListenerChangeListener from Bug 524674 to send over mutations when events change for a known target
Comment 2•9 years ago
|
||
The debugger events pane will need this, too.
Updated•9 years ago
|
Priority: -- → P2
Whiteboard: [devedition-40]
Reporter | ||
Comment 3•9 years ago
|
||
This should be easy once Bug 524674 lands
Whiteboard: [devedition-40] → [devedition-40][difficulty=easy]
Reporter | ||
Comment 4•9 years ago
|
||
(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.
Comment 5•9 years ago
|
||
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?
Updated•9 years ago
|
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
Assignee | ||
Comment 6•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa5c04ff3afa
Assignee | ||
Comment 7•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → jdescottes
Assignee | ||
Comment 8•8 years ago
|
||
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/
Reporter | ||
Comment 9•8 years ago
|
||
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+
Assignee | ||
Comment 10•8 years ago
|
||
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/
Assignee | ||
Comment 11•8 years ago
|
||
New try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=c963ab2efa6c&selectedJob=15139631 Some intermittent or unrelated failures.
Keywords: checkin-needed
Comment 12•8 years ago
|
||
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)
Assignee | ||
Comment 13•8 years ago
|
||
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)
Assignee | ||
Comment 14•8 years ago
|
||
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/
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/d933300edeed
Keywords: checkin-needed
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d933300edeed
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Assignee | ||
Comment 17•8 years ago
|
||
Assignee | ||
Comment 18•8 years ago
|
||
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)
Assignee | ||
Comment 19•8 years ago
|
||
@KWierso : feel free to back this out if it occurs too frequently
Flags: needinfo?(wkocher)
Assignee | ||
Comment 20•8 years ago
|
||
@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)
Comment 21•8 years ago
|
||
Backed out in https://hg.mozilla.org/mozilla-central/rev/d4213241bb79 and merged around.
Status: RESOLVED → REOPENED
status-firefox46:
fixed → ---
Flags: needinfo?(philringnalda)
Resolution: FIXED → ---
Target Milestone: Firefox 46 → ---
Comment 22•8 years ago
|
||
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)
Assignee | ||
Comment 23•8 years ago
|
||
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)
Assignee | ||
Comment 24•8 years ago
|
||
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.
Assignee | ||
Comment 25•8 years ago
|
||
smaug: new stack trace in Comment 24 if you want to have a look
Flags: needinfo?(bugs)
Comment 26•8 years ago
|
||
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)
Comment 27•8 years ago
|
||
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.
Assignee | ||
Comment 28•8 years ago
|
||
@smaug : thanks! new try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=da6aba6da689
Flags: needinfo?(jdescottes)
Assignee | ||
Comment 29•8 years ago
|
||
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
Assignee | ||
Comment 30•8 years ago
|
||
Cancelled https://treeherder.mozilla.org/#/jobs?repo=try&revision=49d14f3b869e (triggered from the wrong changeset) New try push at : https://treeherder.mozilla.org/#/jobs?repo=try&revision=67366b321333
Assignee | ||
Comment 31•8 years ago
|
||
Smaug: your last patch fixes the issue. How do we proceed to land this?
Flags: needinfo?(bugs)
Comment 32•8 years ago
|
||
the patch needs a review. Once it has r+, feel free to land with other patches.
Flags: needinfo?(bugs)
Comment 33•8 years ago
|
||
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+
Comment 34•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8597419 -
Attachment is obsolete: true
Assignee | ||
Comment 35•8 years ago
|
||
Comment on attachment 8706500 [details] [diff] [review] +comments Carrying over the R+ from smaug's last patch
Attachment #8706500 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8705861 -
Attachment is obsolete: true
Assignee | ||
Comment 36•8 years ago
|
||
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+
Assignee | ||
Comment 37•8 years ago
|
||
Try push with the latest patch from smaug applied : https://treeherder.mozilla.org/#/jobs?repo=try&revision=89860813412c&selectedJob=15329672
Assignee | ||
Comment 38•8 years ago
|
||
Last try push was successful, adding checkin-needed for both patches
Keywords: checkin-needed
Comment 39•8 years ago
|
||
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)
Assignee | ||
Comment 40•8 years ago
|
||
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/
Assignee | ||
Comment 41•8 years ago
|
||
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)
Comment 42•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/3791e857a33f https://hg.mozilla.org/integration/fx-team/rev/dd3a10e8fcce
Keywords: checkin-needed
Updated•8 years ago
|
Flags: needinfo?(cbook)
Comment 43•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3791e857a33f https://hg.mozilla.org/mozilla-central/rev/dd3a10e8fcce
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Comment 44•8 years ago
|
||
[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: 1260763
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•