Bug #814889 added a message to warn users that they can’t inspect HTML while debugging: “Debugger is paused. Some features like mouse selection will not work.”. The justification for this was that (1) it’d be hard to fix properly and (2) other browsers also didn’t let you select using the mouse with a paused debugger. I can’t comment on (1) as I don’t know the code, but (2) is no longer the case. Both Chrome and Safari have no problem in using a mouse to inspect a DOM node while the debugger is paused. So I’m filing this bug to reinvestigate whether it’s still as technically demanding as it was 2 years ago. STR: 1. Load a page with a triggerable JS code path. 2. Add a breakpoint in that path in the Debugger tab. 3. Trigger that code path. 4. Click on the Inspector tab.

For what it’s worth, IE11 doesn’t allow selection when the debugger is paused.

I don't know if e10s and the new highlighter implementation have changed the situation here, but maybe Patrick does.

Making the highlighter work while paused would be great. After discussing quickly with Panos last week, I learned that the debugger was somehow blocking DOM events to happen on the page while paused (to avoid javascript from running when mouse events occur for example). I don't know how this works, but it ends up blocking the events the highlighter uses too. For info, the highlighter registers a bunch of events from the page here: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/highlighter.js#380 The pageListenerTarget that is used here to register event listener is defined here: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/highlighter.js#3242 -> contentWindow.docShell.chromeEventHandler. If we want the highlighter to still work while paused, we need a way for events to still reach these listeners.

Devtools asked for events to be suppressed here: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webbrowser.js#1552

Could this bug be marked something else than UNCONFIRMED and maybe get a priority, etc? I am also fairly often slowed down by this: Not being able to pick an element with the mouse within inspector makes debugging very inconvenient/harder - and when does this happen: when you want to debug and have a breakpoint set ;)

I'm not sure this patch does what I want it to do, but that may be a way to make these things to work. nsIDOMWindowUtils.suppressEventHandling totally disable events. I haven't found any way to get notified of the mouse events. addSystemEventListener, addEventListener on chromeEventHandler, on tabChildGlobal, ... nothing get called. But we may be able to add yet another option on top of suppressEventHandling, in order to disable event only for the content, and still let them pass through the chrome layers. Any feedback??

Nothing exploded on try. Just the test asserting suppressEventHandling behavior. It looks like platform tests are using chrome listeners, or, this patch doesn't work as expected. Only one devtools test related to pause on exception fails. We may not have any devtools test asserting event listener disabling...

In order to unlock various limitation in devtools when the debugger is paused, I'm suggesting to make it so that nsIDOMWindowUtils.suppressEventHandling forces dispatching events only to the chrome listeners, while still preventing them to be fired within the content document. It will typically fire on the chromeEventHandler or on the iframe element. Eddy, do you think that's reasonable? TBH, I do not really know what is the real purpose of event cancellation for the debugger... If I get a green light from you, I'll craft a platform patch to allow doing that. See attachment 8806892 [details] [diff] [review] for a gross attempt to verify this solution.

(In reply to Alexandre Poirot [:ochameau] from comment #10) > In order to unlock various limitation in devtools when the debugger is > paused, > I'm suggesting to make it so that nsIDOMWindowUtils.suppressEventHandling > forces dispatching events only to the chrome listeners, while still > preventing them to be fired within the content document. > It will typically fire on the chromeEventHandler or on the iframe element. > > Eddy, do you think that's reasonable? > TBH, I do not really know what is the real purpose of event cancellation for > the debugger... > > If I get a green light from you, I'll craft a platform patch to allow doing > that. > See attachment 8806892 [details] [diff] [review] for a gross attempt to > verify this solution. That's a clever solution, which I didn't consider. Truth be told, I don't fully understand the implications of this change. In theory, not blocking chrome events when the debugger is paused should be fine when debugging content, since pausing the debugger should only block content events anyway. What about browser debugging though? Wouldn't this change make it so that mouse selection keeps working even when the browser debugger is paused? If so, we should probably only set this flag for content debugging. I think it's okay to move ahead with this, but keep the above in mind.

Great! Yes it will still be problematic for the browser toolbox, but it isn't as important. I'll now try to verify that my preliminary patch behaves as expected.

Feedback would be much appreciated from someone knowing highlighters/inspector to check if these two patches fixes all the issues when the debugger is paused. Here is an helpful url to test this: data:text/html,<script>setTimeout(()=>{debugger;}, 1000)</script>foo

I got some green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f4136ae1d3e But there is something I'm unable to address. Even with a bunch of additionnal fixes I pushed to the mozreview request (a third patch). It doesn't freeze everything. The event listeners seem to be disable when set from the content while there are still working when set from the chrome on the chrome event handler. But things like CSS :hover are still working while the debugger is paused. And this goes way beyond my knowledges to figure this out :/ Also I was wondering if we have to bug open to mimic all other browser which gray out the website while the debugger is paused? It may be a workaround...

Here is a test url that shows :hover still working while the debugger is paused: data:text/html,<style>div:hover{background:blue}</style><script>setTimeout(()=>{debugger;}, 1000)</script><div>foo</div>

(In reply to Alexandre Poirot [:ochameau] from comment #20) > Also I was wondering if we have to bug open to mimic all other browser which > gray out the website while the debugger is paused? It may be a workaround... We have bug 1338582 for this. Now, the problem we have is we would like this gray overlay to contain a toolbar that has step over/in/out and resume buttons. But, if events are suppressed, then those buttons can't be clicked!

Without this patch, yes, your buttons won't work if they are part of the document. May be we can spawn another document that would be on top of it? Or have it done from the parent process in the chrome UI? Otherwise, with this patch, the buttons will still work as they are being set from chrome scope. Then it may also help workaround some limitations of this patch. It may prevent dispatching events to content and trigerring any action in the rendering engine that are still being trigerred (like :hover).

Comment on attachment 8811184 [details] Bug 1177346 - Allow dispatching events only to chrome event handlers. r=? :ochameau and I were discussing about this bug on IRC today. :ochameau had done this C++ patch some time ago which would be very helpful to DevTools. The context is that sometimes, when the debugger is paused, we don't want to suppress all events, but only content ones, so that DevTools can continue to handle events at chrome level. The reason we want to do this is because DevTools sometime inject content into the document so it can display things. And we want these things to be interactive. Like, inserting a button that can still be clicked. We'd like to have some feedback on the approach because this is code that we don't know much about. Looking at https://wiki.mozilla.org/Modules/All#Event_Handling, you and :smaug seem to be the only 2 peers, and :smaug has disabled reviews (not sure about feedback). Could you please provide some feedback on this?

Sorry for the delay to reply. I'll try to check it tomorrow.

Comment on attachment 8811184 [details] Bug 1177346 - Allow dispatching events only to chrome event handlers. r=? https://reviewboard.mozilla.org/r/93398/#review130180 ::: dom/base/nsDOMWindowUtils.cpp:1667 (Diff revision 2) > + if (!nsContentUtils::IsCallerChrome()) { > + return NS_ERROR_DOM_SECURITY_ERR; > + } > + > + nsCOMPtr<nsIDocument> doc = GetDocument(); > + NS_ENSURE_TRUE(doc, NS_ERROR_FAILURE); Although, using NS_ENRUE_* isn't invalid, using NS_WARN_IF is better in new code. ::: dom/base/nsIDocument.h:2150 (Diff revision 2) > // Note that suppressing events also suppresses animation frames, so > // there's no need to split out events in its own bitmask. > eEvents = 0x3, > }; > > + virtual void SetEventHandlingOnlyChrome(bool aEnable) { nit: Put |{| to the next line. ::: dom/base/nsIDocument.h:2151 (Diff revision 2) > + mEventsOnlyChrome = aEnable; > + }; I think that this is wrong implementation. I guess that this won't affect to subdocuments but I think they should have same state. ::: dom/base/nsIDocument.h:3269 (Diff revision 2) > // If we're an external resource document, this will be non-null and will > // point to our "display document": the one that all resource lookups should > // go to. > nsCOMPtr<nsIDocument> mDisplayDocument; > > + uint32_t mEventsOnlyChrome; Why uint32_t for this? And move to next to other bool members for not increasing the instance size when you change this to bool. ::: dom/interfaces/base/nsIDOMWindowUtils.idl:960 (Diff revision 2) > + /** > + * Dispatch events only to chrome event handlers in window's document and > + * subdocuments. > + * > + * @throw NS_ERROR_DOM_SECURITY_ERR if called without chrome privileges and > + * NS_ERROR_FAILURE if window doesn't have a document. > + */ > + void eventHandlingOnlyChrome(in boolean aEnable); suppressEventHandlingInContent() could be better for consistency with suppressEventHandling(). However, this is really different from it. Both these two methods can be called same time. So, different name could be better than similar name... ::: layout/base/nsPresShell.cpp:7799 (Diff revision 2) > + // Force dispatching only to chrome handler events using coordinates. > + // Typically when the js debugger is paused. > + if (mDocument->EventHandlingOnlyChrome() || > + (frame && frame->PresContext()->Document()->EventHandlingOnlyChrome())) { > + aEvent->mFlags.mOnlyChromeDispatch = true; > + } > + > // Suppress mouse event if it's being targeted at an element inside > // a document which needs events suppressed > if (aEvent->mClass == eMouseEventClass && > frame->PresContext()->Document()->EventHandlingSuppressed()) { > if (aEvent->mMessage == eMouseDown) { > mNoDelayedMouseEvents = true; > } else if (!mNoDelayedMouseEvents && (aEvent->mMessage == eMouseUp || > // contextmenu is triggered after right mouseup on Windows and right > // mousedown on other platforms. > aEvent->mMessage == eContextMenu)) { > DelayedEvent* event = new DelayedMouseEvent(aEvent->AsMouseEvent()); > if (!mDelayedEvents.AppendElement(event)) { Hmm, should the delayed event be fired only in chrome even after the suppressing is canceled? If no, I think that this should be checked at PresShell::DispatchEventToDOM(). And I wonder, content command event may be handled even during the event is handled. See http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/dom/events/EventStateManager.cpp#5187 http://searchfox.org/mozilla-central/source/dom/events/EventStateManager.cpp#5314

smaug: do you have some comments?

> See > http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/dom/events/EventStateManager.cpp#5187 > http://searchfox.org/mozilla-central/source/dom/events/EventStateManager.cpp#5314 Oops, the latter is: See http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/dom/events/EventStateManager.cpp#5187 http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/dom/events/EventStateManager.cpp#5314

What happens to focus/blur/focusin/out events? But yes, something like this might work. Perhaps tricky to get all the events right this way though. Do we need some global main thread flag telling that we're in debugging mode and if so, EventDispatcher would automatically set the flag to all the events (related to the debugged documents)? Perhaps some global Atomic<bool> telling that we're debugging, and if so, check if we're in main thread and then check the target of the events and try to read the EventHandlingOnlyChrome() value.

Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)

WIP for another attempt to fix this bug. This patch takes a different approach from the previous patches here. My concern with them is that if we allow the debugger to pause without setting document->EventHandlingSuppressed(), most of the other mechanisms for avoiding run-to-completion bugs will break, and any fixes won't gel well with the meaning of document->EventHandlingOnlyChrome(). Instead, this patch adds a back channel for sending events to a document when its event handling is suppressed. window.windowUtils.setSuppressedEventListener() specifies a handler to call with certain events --- only those which the element highlighter needs --- when a document associated with the window has suppressed the event. The element highlighter uses this handler to watch for suppressed events and forward them to its normal event handlers. This takes advantage of the fact that the highlighter only installs its event handlers on one element, keeping this dispatching simple. This WIP is able to highlight elements when hovering over them while paused in the debugger on a simple page. Click events aren't handled yet, and the eyedropper doesn't work either.

Patch which I think allows all inspector functionality for clicking elements and using the eyedropper, and works when selecting elements in subdocuments.

Add IDL for a setSuppressedEventListener interface that can be called through window.windowUtils. The handler passed in will be called with mouse events that were blocked from going to documents in the window because they have events suppressed. This is only used in a couple places in devtools (which should not both be using it simultaneously) so having a single handler instead of a set of handlers (as with normal event listeners) seems preferable. This patch changes nsIDOMEventListener to be scriptable so that the handler can be implemented in JS for part 2. This doesn't seem like the right way of doing this, but I'm not sure what would be better. I tried changing the use of nsIDOMEventListener to EventListener and added a 'webidl EventListener;' declaration at the top, but got a compile error: xptdata.cpp:15:10: fatal error: 'mozilla/dom/EventListener.h' file not found

Use the new setSuppressedEventListener interface to allow the element highlighter and eyedropper to work while the debugger is paused. The only tricky bit here is that events sent to this listener have not been converted into clicks, so we watch for mouseup instead.

Remove the warning shown when using the inspector while paused in the debugger. While I'm not 100% sure there is parity with behavior when not paused, any remaining issues should not be hard to fix.

Comment on attachment 9033143 [details] [diff] [review] Part 1 - Add setSuppressedEventListener interface. >+static bool SetSuppressedEventListenerInSubDocument(nsIDocument* aDocument, void* aData) { >+ aDocument->SetSuppressedEventListener((nsIDOMEventListener*) aData); I'd prefer C++ casting >+static void SetSuppressedEventListenerInAllSubDocuments(nsIDocument* aDocument, void* aData) { >+ aDocument->EnumerateSubDocuments(SetSuppressedEventListenerInSubDocument, aData); >+} Why you need this method. Couldn't SetSuppressedEventListener just call EnumerateSubDocuments(SetSuppressedEventListenerInSubDocument, aListener); >@@ -4051,6 +4058,8 @@ class nsIDocument : public nsINode, > // events were suppressed. > nsTArray<RefPtr<mozilla::net::ChannelEventQueue>> mSuspendedQueues; > >+ RefPtr<nsIDOMEventListener> mSuppressedEventListener; >+ This needs to be traversed/unlinked So add it to https://searchfox.org/mozilla-central/rev/fc229ed2c78648e402a9bbd50d99b69d0e227844/dom/base/nsDocument.cpp#1890 https://searchfox.org/mozilla-central/rev/fc229ed2c78648e402a9bbd50d99b69d0e227844/dom/base/nsDocument.cpp#1799 >+++ b/dom/interfaces/events/nsIDOMEventListener.idl >@@ -15,7 +15,7 @@ webidl Event; > * http://www.w3.org/TR/DOM-Level-2-Events/ > */ > >-[uuid(df31c120-ded6-11d1-bd85-00805f8ae3f4)] >+[scriptable, uuid(df31c120-ded6-11d1-bd85-00805f8ae3f4)] > interface nsIDOMEventListener : nsISupports I'd rather not make this scriptable again, just when we've managed to make it C++ only. You could use .webidl, and add the relevant method as [ChromeOnly] to Document interface and it could take EventListener as param. (unless you figure out how to use EventListener in .idl)

Updated patch per comments.

This is the same as the earlier patch, except that the suppressed event listener is now set on window.document instead of window.windowUtils.

Comment on attachment 9033837 [details] [diff] [review] Part 2 - Use setSuppressedEventListener in highlighter and eyedropper. Review of attachment 9033837 [details] [diff] [review]: ----------------------------------------------------------------- I took some time to build and test this patch, it works great! Thanks a ton for looking in this very old issue. It would be really great if we could have a test with a paused script. To test the key listener you could copy/adapt this test: https://searchfox.org/mozilla-central/source/devtools/client/inspector/flexbox/test/browser_flexbox_highlighter_color_picker_on_ESC.js#44-49 Or this one: https://searchfox.org/mozilla-central/source/devtools/client/inspector/test/browser_inspector_highlighter-cancel.js#33-34 For move over you could copy/adapt this one: https://searchfox.org/mozilla-central/source/devtools/client/inspector/test/browser_inspector_highlighter-03.js#33-46 ::: devtools/server/actors/highlighters.js @@ +390,5 @@ > + this._onPick(event); > + } > + }, > + > + _setSuppressedEventListener(callback) { A JSDoc header for this function would be helpful to explain the platform limitation and why we are using magic chrome-only method. @@ +392,5 @@ > + }, > + > + _setSuppressedEventListener(callback) { > + const document = this._targetActor.window.document; > + document.setSuppressedEventListener(callback ? { handleEvent: callback } : null); It would be handy to comment about why setSuppressedEventListener doesn't accept functions. @@ +405,2 @@ > target.addEventListener("dblclick", this._preventContentEvent, true); > target.addEventListener("keydown", this._onKey, true); I imagine that `preventContentEvent` isn't necessary when the page is paused, right? But the keydown's _onKey listener would require similar fix. When the element selector is enabled, arrow keys don't work when paused. They allow to navigate into the DOM tree easily. ::: devtools/server/actors/highlighters/eye-dropper.js @@ +164,4 @@ > // Focus the content so the keyboard can be used. > this.win.focus(); > > + this.win.document.setSuppressedEventListener(this); Same here, a comment similar to the one you would add in highlighters.js's _setSuppressedEventListener could help understand this. @@ +335,4 @@ > this.moveTo(x, y); > break; > case "click": > + case "mouseup": A comment similar to the one you added in highlighters.js would be helpful here.

Comment on attachment 9033145 [details] [diff] [review] Part 3 - Remove debugger paused warning. Review of attachment 9033145 [details] [diff] [review]: ----------------------------------------------------------------- I'm wondering what we should do with bug 1353564. Do we still want to do an overlay with debugger actions? If not, may be we should remove the pause overlay which we never used: bug 1353564 and https://searchfox.org/mozilla-central/search?q=PausedDebuggerOverlay&case=false&regexp=false&path=

Pushed by bhackett@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6fffc01beb9d Part 1 - Add setSuppressedEventListener interface, r=smaug. https://hg.mozilla.org/integration/mozilla-inbound/rev/bbade19bc70a Part 2 - Use setSuppressedEventListener in highlighter and eyedropper, r=ochameau. https://hg.mozilla.org/integration/mozilla-inbound/rev/23699c5fb0b2 Part 3 - Remove debugger paused warning, r=ochameau. https://hg.mozilla.org/integration/mozilla-inbound/rev/30f3633540ca Part 4 - Add test for using inspector while paused.

https://hg.mozilla.org/mozilla-central/rev/6fffc01beb9d https://hg.mozilla.org/mozilla-central/rev/bbade19bc70a https://hg.mozilla.org/mozilla-central/rev/23699c5fb0b2 https://hg.mozilla.org/mozilla-central/rev/30f3633540ca