1573327 - Ensure breakpoints are hit on load

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement, P2)

Description

[Jason Laster [:jlast]]

The debugger pauses workers temporarily when attaching in order to set breakpoints. We should ensure that we do the same thing for other targets so that breakpoints are hit on load.

Comment 1

[Jason Laster [:jlast]]

Breakpoints definitely work with processes, i have not been able to test the load case, but i'm feeling good about the base case.

Comment 2

[Brian Hackett [Laid off!]]

Looking at this bug, there are some tricky issues. Pausing workers at startup has drawbacks:

• The handshaking to get the worker to pause and unpause at appropriate times is not very simple. Doing this with the main thread of a content process would be even more complicated.

• There is a performance cost where the worker can't start running until the client has attached to it. Doing this with the main thread of a content process could cause noticeable delays while loading.

Currently, the debugger is able to hit breakpoints on load only when reloading. This is because the same actors are maintained in the content process after reloading, with the same installed breakpoints which can be set up on newly encountered sources during the reload. This is kind of nice and it should continue working with fission, except for out of process iframes. In that case a process is running the iframe's content which the debugger has not necessarily attached to and installed breakpoints in. Handling this case seems like it should be simpler than the general problem of allowing the debugger to hit breakpoints on load (i.e. navigations other than reloads). The debugger would just need to make sure the iframe doesn't start loading until the debugger has attached to the main thread of its host process.

Limiting the scope of this bug to iframes would first require debuggers for a browsing context to be able to connect to out of process iframes, however.

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Brian Hackett (:bhackett) from comment #2)

Limiting the scope of this bug to iframes would first require debuggers for a browsing context to be able to connect to out of process iframes, however.

Looking at this again, I think this approach can proceed even within the OBT --- we should be able to pause at breakpoints when an OOP iframe is initially loading, in both the OBT and the content toolbox.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: WIP

This is pretty rough but it does work and follows the approach outlined above. I noticed that the OBT debugger already is able to pause in out of process iframes when the process for that iframe already exists before the iframe has been loaded (i.e. there is another tab or iframe with content from that domain). In this case the OBT has already attached and installed breakpoints, and they will be hit when the new sources appear. What we need to do for the newly-started process case is make sure the OBT has attached to the process before the loading starts.

The best place I could find to pause the load while the devtools attach is before the ContentParent sends a CrossProcessRedirect message for the load, for a few reasons:

• We don't want to intercept the loading process earlier, such as when the content process is spawned, as this won't work very well with content toolboxes. The OBT will attach to all processes so this strategy would work for it, but a content toolbox will only want to attach to the processes relevant to its target tab, and at spawn time it won't know if the process is relevant. When sending the CrossProcessRedirect we have the browsing context which triggered the load.

• After sending the CrossProcessRedirect message there doesn't seem to be another opportunity for the parent process to pause the load. I looked through the IPDL messages sent after this and it seems like the content process triggers all activity that happens until the iframe html is loaded (e.g. constructing its BrowsingContext and WindowGlobal actors), and doesn't need anything else from the parent.

• Calls to send CrossProcessRedirect are already asynchronous, so pausing things when the devtools are present doesn't affect control flow.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: Bug 1573327 Part 1 - Add DebuggerUtils interface that allows devtools to temporarily suspend a cross process redirection, r=nika.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: Bug 1573327 Part 2 - Notify server after the debugger has attached to a process, r=jlast.

Depends on D49806

Comment 7

[Brian Hackett [Laid off!]]

Attachment: Bug 1573327 Part 3 - Block cross process redirects until the client has attached to the target process, r=ochameau.

Depends on D49807

Comment 8

[Brian Hackett [Laid off!]]

Attachment: Bug 1573327 Part 4 - Set watchedByDevtools flag in all docshells for content process targets, r=ochameau.

Depends on D49808

Comment 9

[Brian Hackett [Laid off!]]

Attachment: Bug 1573327 Part 5 - Add test that breakpoints are hit on load in OOP iframes, r=jlast.

Depends on D49810

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 9102669 [details]
Bug 1573327 Part 2 - Notify server after the debugger has attached to a process, r=jlast.

Thanks Brian for looking into this complex scenario! I'm really sorry for not getting back to you sooner but I had a hard time context switching to this complex topic.

Unfortunately, this patch looks wrong for the same reason as https://phabricator.services.mozilla.com/D48265.
Remote frames should be handled by a BrowsingContextTargetActor, instead of a ContentProcessTargetActor/ProcessDescriptor.
It isn't clear to me how we are going to make this work, once Julian is done with JS Window Actor API and we start using BrowsingContextTarget actors for debugging remote frames.
I'm especially wondering about this call:
await debuggerClient.mainRoot.processAttached(targetFront.processID);
Where I would rather expect something like:
targetFront.processAttached() or targetFront.descriptorFront.processAttached()
Which makes me think that this code should be inlined into the existing "attach" codepath, (i.e TargetMixin.attach() / BrowsingContextTargetActor.attach() / ...), or within the Descriptor.getTarget, which is calling these attach method:
https://searchfox.org/mozilla-central/source/devtools/shared/fronts/descriptors/process.js#71-72

In the expected setup, targetFront will be a BrowsingContextTargetActor, running in the content process and once bug 1579042 lands, targetFront.descriptorFront will be a TabDescriptor, running in the parent process.

I think you are a bit ahead of schedule and this will be easier to implement once Julian and Yulia's refactorings will be completed.
Nonetheless, this is a very interesting approach and certainly rises some other challenges regarding how to implement early breakpoints.

For example:

• I received some questions from DOM Fission group about why SendCrossProcessRedirectNotifyDevtools was async?
  I imagine that there is many reasons, including the fact that RDP is async. But It would be useful to ensure that we can't react synchronously to this event. I received concerns about us delaying the load asynchronously and possibly introduce races or drastically change the behavior of the page load.
  I was suggested to restrict the async to the bare minimum, if we really had to be async. If I follow the current patch queue, it looks like we delay the load a lot, including the load of a bunch of frontend bootstrap. Do you think we could possibly synchronously load the bare minimum: a DebuggerServer + Target Actor + Thread Actor + breakpoints?
• This patch is specific to early breakpoints for iframes. But I'm wondering what we would be doing for early breakpoints in chrome code, like JSM or XPCOM is JS? This should actually be possible to implement with existing actors, as I'm expecting this to involve the ContentProcessTargetActor and ProcessDescriptor. Such work would actually better align with our current goals being focused around the Omniscient Browser Toolbox rather than Fission frames.
• Note that the fact that DebuggerUtils.crossProcessRedirectCallback is global could be an issue if two DevTools are running at the same time. Like a regular web toolbox and a browser toolbox. I would expect the one loaded first to have its promise being overloaded by the devtools loaded second. I imagine that two regular web toolboxes would also collide in a similar way?

Comment 11

[Brian Hackett [Laid off!]]

(In reply to Alexandre Poirot [:ochameau] from comment #10)

Comment on attachment 9102669 [details]
Bug 1573327 Part 2 - Notify server after the debugger has attached to a process, r=jlast.

Thanks Brian for looking into this complex scenario! I'm really sorry for not getting back to you sooner but I had a hard time context switching to this complex topic.

Unfortunately, this patch looks wrong for the same reason as https://phabricator.services.mozilla.com/D48265

Remote frames should be handled by a BrowsingContextTargetActor, instead of a ContentProcessTargetActor/ProcessDescriptor.
It isn't clear to me how we are going to make this work, once Julian is done with JS Window Actor API and we start using BrowsingContextTarget actors for debugging remote frames.

I'm especially wondering about this call:
await debuggerClient.mainRoot.processAttached(targetFront.processID);
Where I would rather expect something like:
targetFront.processAttached() or targetFront.descriptorFront.processAttached()

Sure, this would be fine, we just need to send an RDP message from the toolbox process to the parent process that lets it know to unblock a cross process redirect. Right now this is done as a processAttached(pid) message. When the OBT debugger starts connecting to browsing context targets instead of content processes, this message would change to browsingContextAttached(browsingContextId). A message still needs to be sent though, as the client in the toolbox is in a different process from where the DebuggerUtils.crossProcessRedirectCallback callback lives.

When we are doing remote frame debugging from the content toolbox, the client lives in the parent process, and can manipulate DebuggerUtils.crossProcessRedirectCallback directly. Further JS changes will be necessary to get the remote frame case working in the content toolbox, but they will be simpler.

Which makes me think that this code should be inlined into the existing "attach" codepath, (i.e TargetMixin.attach() / BrowsingContextTargetActor.attach() / ...), or within the Descriptor.getTarget, which is calling these attach method:
https://searchfox.org/mozilla-central/source/devtools/shared/fronts/descriptors/process.js#71-72

Maybe, I'll experiment to see if the debugger is ready when the target attaches, as there is extra thread-attaching logic in attachTargets(). The message here might be better named as debuggerReady() or something instead of processAttached().

• I received some questions from DOM Fission group about why SendCrossProcessRedirectNotifyDevtools was async?
  I imagine that there is many reasons, including the fact that RDP is async. But It would be useful to ensure that we can't react synchronously to this event. I received concerns about us delaying the load asynchronously and possibly introduce races or drastically change the behavior of the page load.
  I was suggested to restrict the async to the bare minimum, if we really had to be async. If I follow the current patch queue, it looks like we delay the load a lot, including the load of a bunch of frontend bootstrap. Do you think we could possibly synchronously load the bare minimum: a DebuggerServer + Target Actor + Thread Actor + breakpoints?

I don't know how we could do this synchronously: these operations require communicating with another process over a pipe and are fundamentally asynchronous. It's possible we could do fewer asynchronous operations to set up the connection, but that would only affect performance and not correctness, and would be better to optimize later.

• This patch is specific to early breakpoints for iframes. But I'm wondering what we would be doing for early breakpoints in chrome code, like JSM or XPCOM is JS? This should actually be possible to implement with existing actors, as I'm expecting this to involve the ContentProcessTargetActor and ProcessDescriptor. Such work would actually better align with our current goals being focused around the Omniscient Browser Toolbox rather than Fission frames.

I don't think that supporting early breakpoints in chrome code is relevant to fission, and it's not something we currently support. As outlined in comment 2, the handshaking to get a piece of code to avoid executing until the devtools have attached is rather tricky to get right, and has performance implications. I'm focusing on iframes because that is what is needed to bring the post-fission debugger to parity with the pre-fission debugger. If we want to get other early-breakpoint cases working then I think it would be better done in separate, non-fission-blocking bugs.

• Note that the fact that DebuggerUtils.crossProcessRedirectCallback is global could be an issue if two DevTools are running at the same time. Like a regular web toolbox and a browser toolbox. I would expect the one loaded first to have its promise being overloaded by the devtools loaded second. I imagine that two regular web toolboxes would also collide in a similar way?

If there are multiple toolboxes which want to install a callback, it will only be installed by the first one which attempts to, per the code in part 3. This ensures that no toolbox overrides an existing toolbox's callback, and that things will behave in the expected fashion for that first toolbox. The second toolbox will not be able to block cross process redirects. This could be improved so that the toolboxes coordinate with each other so that the redirect doesn't happen until they are all ready for it, but the extra complexity didn't seem worth it.

Comment 12

[Alexandre Poirot [:ochameau]]

(In reply to Brian Hackett (:bhackett) from comment #11)

Sure, this would be fine, we just need to send an RDP message from the toolbox process to the parent process that lets it know to unblock a cross process redirect. Right now this is done as a processAttached(pid) message. When the OBT debugger starts connecting to browsing context targets instead of content processes, this message would change to browsingContextAttached(browsingContextId). A message still needs to be sent though, as the client in the toolbox is in a different process from where the DebuggerUtils.crossProcessRedirectCallback callback lives.

Yes, it makes sense if we have to be async.
But this is really disturbing as you are handling the browsing context usecase into the content process codepath.

ContentProcessTarget aren't about the remote frames. A content process isn't bound to any given document. It isn't bound to a single BrowsingContext. It can contain some, but content processes are much more generic.
The "browsing contexts" and BrowsingContextTarget are about the remote frames.
So it feels really weird to freeze the content processes / ContentProcessTarget for a BrowsingContext usecase.

When we are doing remote frame debugging from the content toolbox, the client lives in the parent process, and can manipulate DebuggerUtils.crossProcessRedirectCallback directly. Further JS changes will be necessary to get the remote frame case working in the content toolbox, but they will be simpler.

The client should never interact with gecko directly, except for UI/Frontend integration.
If the client was to interact directly with gecko, it means that it would break when debugging remote targets (Firefox Mobile).
But may be I misunterstood your point here?

I don't know how we could do this synchronously: these operations require communicating with another process over a pipe and are fundamentally asynchronous. It's possible we could do fewer asynchronous operations to set up the connection, but that would only affect performance and not correctness, and would be better to optimize later.

I've received a different feedback about that. Nika told me since Berlin that the IPC messages order is important, and that it we could depend on this to implement early breakpoints.
If I got it correctly, the first message queued is the first message sent and the first to be received on the other end.
So that if we delay CrossProcessRedirect, we most likely shuffle the order of messages being queued and so shuffle the order of messages received in the content process.
If we were able to synchronously queue a DevTools specific message, right after CrossProcessRedirect, we would only insert a new message in the queue and prevent having CrossProcessRedirect to be sent and received in a different order compared to what it is without DevTools.

Now, I'm also having a hard time seeing how we can be fully synchronous. If we have to involve a RDP event or method call, it will be async.
But I was wondering what were the fundamental actions that we have to execute in the content process and/or the data required to execute these actions?
For example, if we were having the breakpoint always available in the parent process... could we spawn the target actor and attach the thread actor synchronously?

Do you want me to plan a meeting with Nika to go over this?

Otherwise, this patch queue complexifies a bit more the already over-confusing "attach" sequence of DevTools.
By adding yet another "attached" sequence, and I would like to ensure we are going somewhere.
The worker codepath is already different from all others and you are introducing yet another complex pattern for browsing context into the content process targets.
We can probably converge into a unified way when it comes to:

• watch and block target initialization
• create the target actor and front
• attach the console and thread actors and fronts
• release the blocked target initialization

I don't think that supporting early breakpoints in chrome code is relevant to fission, and it's not something we currently support. As outlined in comment 2, the handshaking to get a piece of code to avoid executing until the devtools have attached is rather tricky to get right, and has performance implications. I'm focusing on iframes because that is what is needed to bring the post-fission debugger to parity with the pre-fission debugger. If we want to get other early-breakpoint cases working then I think it would be better done in separate, non-fission-blocking bugs.

You jumped into the DevTools Fission initiative very late, so you clearly missed the fact that the OBT isn't stricly about Fission remote frames.
Instead it is meant to support content processes, similarly to the "Browser Content Toolbox".
So, in the context of the OBT, I would expect the OBT to start setting breakpoints as early as ipc:content-created fires, or even earlier.
So that we can set breakpoint in any JS piece of code used in the content processes: JSM, XPCOM in JS... and the document's JS.
If the OBT was attaching the thread actor that early, it would also be able to break early in remote frames.

Now, I undesrstand that this is very confusing as this bug is on the M1 list and is about the BrowsingContext usecase.
In the beginning of M1 I thought that we would have been done with JSWindowActor API and BrowsingContextTarget integration much earlier... and that the Debugger Team would have been able to start working on the "real fission", using listRemoteFrames. But this has been delayed and makes it hard to implement this one bug right now.
Nonetheless, I believe that your invertigation is really helpful and is certainly going to help implement the early breakpoints for remote frames.

• Note that the fact that DebuggerUtils.crossProcessRedirectCallback is global could be an issue if two DevTools are running at the same time. Like a regular web toolbox and a browser toolbox. I would expect the one loaded first to have its promise being overloaded by the devtools loaded second. I imagine that two regular web toolboxes would also collide in a similar way?

If there are multiple toolboxes which want to install a callback, it will only be installed by the first one which attempts to, per the code in part 3. This ensures that no toolbox overrides an existing toolbox's callback, and that things will behave in the expected fashion for that first toolbox. The second toolbox will not be able to block cross process redirects. This could be improved so that the toolboxes coordinate with each other so that the redirect doesn't happen until they are all ready for it, but the extra complexity didn't seem worth it.

If two regular web toolboxes are impacted by this, I think that we should address that.
But this may be addressed by being synchronous... or less async.

Comment 13

[Alexandre Poirot [:ochameau]]

(In reply to Alexandre Poirot [:ochameau] from comment #12)

The worker codepath is already different from all others and you are introducing yet another complex pattern for browsing context into the content process targets.

About that. I'm wondering why worker and processes are having such different way to implement early breakpoints?
Is there any argument to have distinct API from the DevTools Frontend side?
Only processes require to call a RDP request to resume the load, while worker seems to not require anything.

Comment 14

[Alexandre Poirot [:ochameau]]

Comment on attachment 9102669 [details]
Bug 1573327 Part 2 - Notify server after the debugger has attached to a process, r=jlast.

About this particular patch.
We do call await debuggerClient.mainRoot.processAttached(targetFront.processID); only from the Debugger.
Does that mean that the remote frames will be frozen unless the debugger panel is opened?

Comment 15

[Brian Hackett [Laid off!]]

(In reply to Alexandre Poirot [:ochameau] from comment #12)

(In reply to Brian Hackett (:bhackett) from comment #11)

Sure, this would be fine, we just need to send an RDP message from the toolbox process to the parent process that lets it know to unblock a cross process redirect. Right now this is done as a processAttached(pid) message. When the OBT debugger starts connecting to browsing context targets instead of content processes, this message would change to browsingContextAttached(browsingContextId). A message still needs to be sent though, as the client in the toolbox is in a different process from where the DebuggerUtils.crossProcessRedirectCallback callback lives.

Yes, it makes sense if we have to be async.
But this is really disturbing as you are handling the browsing context usecase into the content process codepath.

ContentProcessTarget aren't about the remote frames. A content process isn't bound to any given document. It isn't bound to a single BrowsingContext. It can contain some, but content processes are much more generic.
The "browsing contexts" and BrowsingContextTarget are about the remote frames.
So it feels really weird to freeze the content processes / ContentProcessTarget for a BrowsingContext usecase.

We're not freezing the content process, we're blocking the cross process redirect. That redirect is associated with both a content process target and a browsing context. The callback allows the devtools to block the redirect until it has finished preparing for the redirected frame to start executing, but what it means to prepare is left up to the devtools. Because the OBT debugger currently attaches to processes, we wait until the debugger has finished attaching to that process. When the OBT debugger attaches to browsing contexts, we will wait until the debugger has finished attaching to the browsing context.

I picked this spot to block the load precisely because both the content process and the browsing context are available at this point, and because this is the last time at which the parent process has an opportunity to block the iframe loading while the devtools are operating. After the CrossProcessRedirect message is sent, the content process will proceed with the load on its own accord without waiting on anything from the parent.

The browsing context ID is not currently supplied to the devtools, but it could be. Earlier versions of part 1 passed it to the devtools as an argument to crossProcessRedirectCallback, but there was confusion about what browsing context needs to be passed and so it seemed better to hold off on that until the devtools are actually able to consume it.

When we are doing remote frame debugging from the content toolbox, the client lives in the parent process, and can manipulate DebuggerUtils.crossProcessRedirectCallback directly. Further JS changes will be necessary to get the remote frame case working in the content toolbox, but they will be simpler.

The client should never interact with gecko directly, except for UI/Frontend integration.
If the client was to interact directly with gecko, it means that it would break when debugging remote targets (Firefox Mobile).
But may be I misunterstood your point here?

No, you're right, I'm off base here. I was thinking that if the client needs to access or manipulate gecko data in its own process, it needs to access that data directly. But that is wrong, it looks like the client is able to communicate with actors in its own process, which for example we need to do in https://phabricator.services.mozilla.com/D47542 to allow a content toolbox client in the parent process to access service worker registrations and connect to the worker threads themselves.

With that functionality, what we can do here is always use a client->server message to notify that the debugger is ready for a cross process redirect to happen. In the browser toolbox case the actor will be in another process, while in the content toolbox case the actor will be in the current process, but it doesn't really make a difference to the debugger.

I don't know how we could do this synchronously: these operations require communicating with another process over a pipe and are fundamentally asynchronous. It's possible we could do fewer asynchronous operations to set up the connection, but that would only affect performance and not correctness, and would be better to optimize later.

I've received a different feedback about that. Nika told me since Berlin that the IPC messages order is important, and that it we could depend on this to implement early breakpoints.
If I got it correctly, the first message queued is the first message sent and the first to be received on the other end.
So that if we delay CrossProcessRedirect, we most likely shuffle the order of messages being queued and so shuffle the order of messages received in the content process.
If we were able to synchronously queue a DevTools specific message, right after CrossProcessRedirect, we would only insert a new message in the queue and prevent having CrossProcessRedirect to be sent and received in a different order compared to what it is without DevTools.

Now, I'm also having a hard time seeing how we can be fully synchronous. If we have to involve a RDP event or method call, it will be async.
But I was wondering what were the fundamental actions that we have to execute in the content process and/or the data required to execute these actions?
For example, if we were having the breakpoint always available in the parent process... could we spawn the target actor and attach the thread actor synchronously?

Do you want me to plan a meeting with Nika to go over this?

I think a meeting would be good to talk about this because there's been a fair amount of discussion about this bug that I wasn't included in.

Introducing a delay that could cause messages to be sent out of order is definitely a big concern on the platform side. I don't think it's a concern for CrossProcessRedirect though, as this triggers a load in the target process rather than (say) being part of the messages used to set up the target process itself. See also Nika's comment in https://phabricator.services.mozilla.com/D49806. The platform changes have already been approved, however, and I don't know why we're relitigating them.

Sending a single async IPDL message before the CrossProcessRedirect message would be good for performance, but I think it would make things very complicated on the server side. The breakpoints are available in the parent process (they're in local storage) and could be sent in the IPDL message, but what would the content process do with them? There is no DebuggerServer yet and no channel for communicating with the parent, no actors or anything. It would need to create a debugger all on its own, install breakpoints, and pause at them. This behavior is currently an intricate part of the thread actor, so we would need to create a dummy thread actor and morph it into the real thread actor later on, factor this logic out and be able to hand off the pause later, or something similarly difficult. Maybe we'll eventually want to do this, but I don't see the point of creating more work for ourselves when we can get something simple working now, even if it isn't as efficient as it could be.

You jumped into the DevTools Fission initiative very late, so you clearly missed the fact that the OBT isn't stricly about Fission remote frames.
Instead it is meant to support content processes, similarly to the "Browser Content Toolbox".
So, in the context of the OBT, I would expect the OBT to start setting breakpoints as early as ipc:content-created fires, or even earlier.
So that we can set breakpoint in any JS piece of code used in the content processes: JSM, XPCOM in JS... and the document's JS.
If the OBT was attaching the thread actor that early, it would also be able to break early in remote frames.

I know that the OBT is doing something that we weren't doing pre-fission, but that doesn't mean we need to pile a bunch of extra work onto this project. This goal isn't explicitly listed in the PRD document and I already outlined the difficulties of doing this in comment 2.

If two regular web toolboxes are impacted by this, I think that we should address that.
But this may be addressed by being synchronous... or less async.

OK, but can this wait for followup? This is a very obscure use case (two toolboxes on the same tab) and it doesn't need to work perfectly right off the bat. The impact here is pretty minor, the second toolbox just doesn't get the benefit of being able to hit early breakpoints in the iframe's load.

The presence of multiple toolboxes wanting to connect to the same iframe makes the prospect of doing this via a single IPDL message even more difficult.

Comment 16

[Brian Hackett [Laid off!]]

(In reply to Alexandre Poirot [:ochameau] from comment #13)

(In reply to Alexandre Poirot [:ochameau] from comment #12)

The worker codepath is already different from all others and you are introducing yet another complex pattern for browsing context into the content process targets.

About that. I'm wondering why worker and processes are having such different way to implement early breakpoints?
Is there any argument to have distinct API from the DevTools Frontend side?
Only processes require to call a RDP request to resume the load, while worker seems to not require anything.

In the worker case, we are able to unblock the load in the same process where the thread attach occurs, the content process. In the cross process redirect case, the load is unblocked in the parent process, while the attach occurs in the content process. Some IPC is required to coordinate these activities. Unblocking the load in the content process wouldn't be appropriate because there is no devtools presence in the content process when the load is first triggered, and thus nothing that can do the initial block. In the worker case, there is already a devtools presence in the process after the devtools attach to the main thread; these devtools create a PauseMatchingWorkers instance which can be used to prevent new workers from running their main script until their thread has been attached to.

Comment 17

[Brian Hackett [Laid off!]]

(In reply to Alexandre Poirot [:ochameau] from comment #14)

Comment on attachment 9102669 [details]
Bug 1573327 Part 2 - Notify server after the debugger has attached to a process, r=jlast.

About this particular patch.
We do call await debuggerClient.mainRoot.processAttached(targetFront.processID); only from the Debugger.
Does that mean that the remote frames will be frozen unless the debugger panel is opened?

Oops, yes, I'll update the patch so this case works.

Comment 18

[Brian Hackett [Laid off!]]

For anyone wondering, I've been kicked off the fission project and will not be working on this or other fission stuff anymore.

Comment 19

[Chris Peterson [:cpeterson]]

Tracking Fission DevTools bugs for Fission Nightly (M6)

Comment 20

[Chris Peterson [:cpeterson]]

Tracking dt-fission-m2-mvp bugs for Fission Nightly milestone (M6c)

Comment 21

[Alexandre Poirot [:ochameau]]

Bug 1620243 is introducing the first usage of the Watcher Actor to listen for a resource (console messages) from the server side.
This demonstrates how to use JS Window Actor API and sharedData object in order to communicate data across processes.
That patch only transfers the Target and Ressource Types to be listened to, but here, we would need to also transfer some panel specific data that actually comes from the user input:

• breakpoints
• DOM breakpoints
• blocked requests
• console/network persists on/off
• ...

The idea would be to have a method on the Watcher Actor, which allows to update this data and have the actor to replicate the data to all processes and thread.

So, instead of doing something similar to this:

for(const target of targetList.getAllTargets()) {
  await target.reconfigure({ breakpoints });
}

We would do, something somewhat like this:

const wacher = await resourceWatcher.getWatcher();
watcher.setData("breakpoints", breakpoints);

Instead of having to call one update method per target, we only call one, the watcher method.
The namings and precise API is still be be discussed.

Comment 22

[Alexandre Poirot [:ochameau]]

What I described in comment 21 would only work if all target types (process, frame and worker) are supported by the watcher actor.
Otherwise, it means that we would still have to call target.reconfigure or target.setBreakpoint for all targets not supported by the watcher.

So I'm making this bug depend on bug 1608848.

Comment 23

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1573327 - [devtools] Pass breakpoints via the Watcher actor.

Comment 24

[Neha Kochar [:neha]]

Bulk move of all m2-mvp devtools bugs to Fission M7.

Comment 25

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1573327 - [devtools] Introduce "commands" in order to ease calling global commands throught the Watcher.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/28b974a96336
[devtools] Pass breakpoints via the Watcher actor. r=jdescottes,devtools-backward-compat-reviewers,nchevobbe

Comment 27

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/28b974a96336

Comment 28

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1573327 - [devtools] Move getBlockRequest to Network command.

Comment 29

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1573327 - [devtools] Use Inspector command for suggestion queries.

Comment 30

[Phabricator Automation]

Comment on attachment 9188831 [details]
Bug 1573327 - [devtools] Introduce "commands" in order to ease calling global commands throught the Watcher.

Revision D97575 was moved to bug 1691681. Setting attachment 9188831 [details] to obsolete.

Comment 31

[Phabricator Automation]

Comment on attachment 9199609 [details]
Bug 1573327 - [devtools] Move getBlockRequest to Network command.

Revision D103203 was moved to bug 1691681. Setting attachment 9199609 [details] to obsolete.

Comment 32

[Phabricator Automation]

Comment on attachment 9199610 [details]
Bug 1573327 - [devtools] Use Inspector command for suggestion queries.

Revision D103204 was moved to bug 1691681. Setting attachment 9199610 [details] to obsolete.