1054159 - Debugger doesn't break on the "load" event

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Trevor Rowbotham [:rowbot]]

Attachment: break_on_event_testcase.html

STR:
1) Open attached test case.
2) Open debugger and go to events pane.
3) Tell the debugger to break on the load event.
4) Refresh page.

Actual Results:
Debugger does not break on the load event.

Expected Results:
Debugger should break on the load event.

Using the same STR as above, but replacing steps 3 and 4 with the following:
3) Tell the debugger to break on the click event.
4) Click anywhere on the page.

You will see that the debugger does break on the event as expected.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

The problem is that the time we add the listener for all events after the reload is too late to observe the load event. I'm working on a patch.

Comment 2

[Trevor Rowbotham [:rowbot]]

At the time that I had filed this bug, breaking on a whole bunch of other events weren't working either like the media events loadstart, loadedmetadata, loadeddata, and waiting, but it would break on events like play, pause, and click.  However, it would seem that the problem I originally reported has been fixed within the past 2 weeks. It would appear that not breaking on the load event is a separate problem from what I was originally reporting, it just happened to exhibit the same problems.  At least my bug report wasn't a total loss.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Fix breaking on the "load" event in the debugger

This small change fixes the problem. I still need to verify that it does the right thing in iframes and add a test.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Thanks for the bug report, it was very helpful indeed! Adjusting the bug title to better reflect the issue.

Comment 5

[Trevor Rowbotham [:rowbot]]

I was just thinking.  I assume that since the debugger was too late in listening for the "load" event that it would also have the same problem with events, such as "DOMContentLoaded", that are dispatched before the "load" event.  Does this patch cover those earlier events as well?

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Fix breaking on the "load" event in the debugger

Here is a rebased patch with a test.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8576694 [details] [diff] [review]
Fix breaking on the "load" event in the debugger

This patch fixes the pause on load events, but not on DOMContentLoaded events. We don't even seem to display those in the Events pane. I'll file a followup for that to investigate.

Comment 8

[Alexandre Poirot [:ochameau]]

Comment on attachment 8576694 [details] [diff] [review]
Fix breaking on the "load" event in the debugger

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

The fix idea looks good, but I wish we could listen for TabActor events from ThreadActor instead of hacking into TabActor!

::: toolkit/devtools/server/actors/webbrowser.js
@@ +2170,5 @@
> +      // If we are tasked with breaking on the load event, we have to add the
> +      // listener early enough.
> +      this._tabActor.threadActor._maybeListenToEvents({
> +        pauseOnDOMEvents: this._tabActor.threadActor._pauseOnDOMEvents
> +      });

I would really like to stop adding direct call to threadActor (or any tab actor) from webbrowser.js and instead ThreadActor to listen to events like will-navigate/navigate/window-ready/window-destroyed.
`will-navigate` may be too early, `window` may be the old inner window and not receive the DOM events for the new document? (or if that's the outer window it may receive them).
`navigate` is obviously too late as it related to load event.
`window-ready` may be a good candidate as it is dispatched during DOMWindowCreated, as soon as the global window object is created, before any page script is loaded. So it sounds like it should be dispatched before DOMContentLoaded/load events.

See how inspector.js or memory.js listen to these events.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

This refactoring though is the purpose of bug 997119, right? Are you suggesting this patch should fix both bugs?

Comment 10

[Alexandre Poirot [:ochameau]]

(In reply to Panos Astithas [:past] from comment #9)
> This refactoring though is the purpose of bug 997119, right?

Yes.

> Are you suggesting this patch should fix both bugs?

No. bug 997119 is about existing code, but I would like to prevent adding new code from ThreadActor within TabActor. That, to lower the amount of work in this refactoring.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Fix breaking on the "load" event in the debugger v2

OK, this version doesn't touch the tab actor at all and makes the thread actor listen for window-ready instead.

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/337e3aa22117

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/337e3aa22117