Closed Bug 1588007 Opened 4 months ago Closed 3 months ago

Cannot register a fetch event listener breakpoint

Categories

(DevTools :: Debugger, enhancement, P3)

enhancement

Tracking

(firefox72 fixed)

RESOLVED FIXED
Firefox 72
Tracking Status
firefox72 --- fixed

People

(Reporter: ehsan, Assigned: chujunlu)

Details

(Keywords: good-first-bug)

Attachments

(1 file)

Thank you for helping make Firefox better. If you are reporting a defect, please complete the following:

What were you doing?

Please tell us what site you were on, and what steps led to the error you are reporting

  1. Go to https://www.youtube.com/
  2. Go to about:debugging, This Nightly, select Youtube's service worker, click on Inspect.
  3. Select Debugger, expend Event Listener Breakpoints, in the search box type "fetch".

What happened?

No results found.

What should have happened?

I should be able to register a fetch event listener breakpoint.

How difficult would this be Logan? I recall we were having a problem with fetch and images; is that still a problem?

Flags: needinfo?(loganfsmyth)

I think this would be pretty straightforward to do. I think all we need is to add

{
    name: "Service Worker",
    items: [
      globalEvent("serviceworker", "fetch"),
    ]
}

to devtools/server/actors/utils/event-breakpoints.js.

The main thing is that this would also trigger if a fetch event were dispatched on a Window or any other global. We don't currently differentiate the type of global at all. That may be fine though, since it's not an event name I'd expect to appear anywhere else anyway.

One other concern could potentially be that we have a Worker section already, but that's actually about the Worker object not the worker global, which could be vaguely confusing for users, but it is what it is.

Flags: needinfo?(loganfsmyth)
Priority: -- → P3

The main thing is that this would also trigger if a fetch event were dispatched on a Window or any other global. We don't currently differentiate the type of global at all. That may be fine though, since it's not an event name I'd expect to appear anywhere else anyway.

Having a new Service Worker section would make sense as they offer more events like onactivate, and oninstall. They also have message which should be already covered by Worker. I'd be also fine with merging them under Worker for now, as search will make them discoverable.

David, given Logan's guidance, this seems to be a good-first-bug?

Flags: needinfo?(dwalsh)

I'd be afraid to tell someone that anything server-related is a good first bug.

Flags: needinfo?(dwalsh)

:logan, seems like this would be a quick fix for you?

Flags: needinfo?(loganfsmyth)

I can if needed, but I agree with Jason that this seems like a good first bug. It may be in the server, but it's a relatively minor change and could be a good first task for someone looking to get into the server code a bit.

Flags: needinfo?(loganfsmyth)

Note that there is special handling for "fetch" in ServiceWorkers that might need/want some special-casing. Specifically, our implementation and the spec track whether a "fetch" listener was added during install and this information is saved and used to determine whether to send "fetch" events to the ServiceWorker. Fetch events will never be dispatched if a handler wasn't added during install time. If, at subsequent runtimes, a "fetch" handler is added, we generate a warning:

With the way that event listener breakpoints are implemented, that doesn't sound like something that would be an issue thankfully.

From Harald's comment, it seems there're more events to add, hence making a service worker section.
I didn't find a related test in server. Let me know if I miss any!

Pushed by loganfsmyth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ffd9d340f304
Add fetch event listener breakpoint for service worker r=loganfsmyth
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 72
Assignee: nobody → chujunlu
You need to log in before you can comment on or make changes to this bug.