Closed Bug 1256397 Opened 5 years ago Closed 3 years ago

Partially convert ThreadActor to protocol.js


(DevTools :: Debugger, defect, P3)



(Not tracked)



(Reporter: ejpbruel, Assigned: ejpbruel)


(Blocks 1 open bug)



(3 files)

No description provided.
Duplicate of this bug: 1235370
The debugger protocol specifies two meaningful fields for error packets: the 'error' field, which specifies that this is an error packet, and indicates the name/type of the error, and the 'message' field, which contains a human readable description of the error.

The only way to send an error packet in protocol.js right now is by throwing an error. Protocol.js will stringify whatever value you throw and use that as the 'message' field for the error packet. Unfortunately, for the name field, it always uses 'unknownError'.

Many of the script actors have a meaningful 'error' field for their error packets, so we shouldn't just replace these with 'unknownError' (I previously did so for SourceActor and EnvironmentActor: that was a mistake, which I intend to rectify in this bug).

To avoid this, I want to propose a minor extension to protocol.js, where instead of stringifying the thrown value, it looks for 'error' and 'message' fields on the thrown object (in practice, we always throw an object as error, so this should be safe), and uses those for the 'error' and 'message' fields of the error packet, respectively.
Attachment #8730331 - Flags: review?(jryans)
ThreadActor is by far the most complicated script actor we have, so unlike the other script actors, I'm going to refactor it in steps.

The first step will be to convert ThreadActor to an instance of ActorClass, without changing any of the request handlers into protocol.js methods yet.

A clever use of object.merge allows us to convert these handlers one at a time (basically, object.merge overrides the fields of requestTypes as defined by ActorClass, and points them to the old request handlers. As we start converting the old request handlers to protocol.js methods, we stop overriding their requestTypes field, so that we will use the version defined by ActorClass instead).
Attachment #8730333 - Flags: review?(jryans)
Attachment #8730333 - Attachment description: patch → Convert ThreadActor into an instance of ActorClass.
This patch converts ThreadActor.prototype.onAttach to a protocol.js method.

This is surprisingly tricky, due to our use of nested event loops here. I've added a long comment to the patch to explain the problem, and how I work around it. I hope it makes sense.
Attachment #8730334 - Flags: review?(jryans)
This is mainly cleanup of the debugger server, so it should be P3.
Priority: -- → P3
Comment on attachment 8730331 [details] [diff] [review]
Protocol.js error packets should have meaningful `error` and `message` fields.

Review of attachment 8730331 [details] [diff] [review]:

I think this seems fine.  

It does slightly change the error messages, since error.toString() would print " e.message", but the error name doesn't really convey much information to begin with.
Attachment #8730331 - Flags: review?(jryans) → review+
Comment on attachment 8730333 [details] [diff] [review]
Convert ThreadActor into an instance of ActorClass.

Review of attachment 8730333 [details] [diff] [review]:

::: devtools/server/actors/script.js
@@ +2015,2 @@
> +ThreadActor.prototype.requestTypes = object.merge(ThreadActor.prototype.requestTypes, {

I think you can use Object.assign and drop the SDK import.
Attachment #8730333 - Flags: review?(jryans) → review+
I'll have to keep thinking about the last patch.
Comment on attachment 8730334 [details] [diff] [review]
Convert ThreadActor.prototype.onAttach to a protocol.js method.

Review of attachment 8730334 [details] [diff] [review]:

Okay, I tried to think of other approaches, but I believe you've convinced me we need something like this. :)

Please add a section to the protocol.js docs[1] about this new ability.


::: devtools/server/protocol.js
@@ +1080,5 @@
> +        // request handlers that actually need it (because they enter a nested
> +        // event loop). For the sake of backwards compatibility, all other
> +        // request handlers can either return their result directly, or as a
> +        // promise, which will be used to resolve the promise we queued earlier.
> +        if (spec.useCallback) {

I think just `callback` would better align with naming of other options.
Attachment #8730334 - Flags: review?(jryans) → feedback+
Try run for "Protocol.js error packets should have meaningful `error` and `message` fields.":
Whiteboard: leave-open
Your patch accidentally appeared in bug 1253976.
Flags: needinfo?(ejpbruel)
(In reply to arni2033 from comment #11)
> Your patch accidentally appeared in bug 1253976.

Sorry about that! Do you need me to do anything to prevent further confusion?
Flags: needinfo?(ejpbruel)
Try push for "Convert ThreadActor into an instance of ActorClass":
No confusion, I was only worried about development process.
If there's no real breakage - then just hide my comments as spam.
Whiteboard: leave-open → [devtools-html] leave-open
No longer blocks: devtools-html-phase2
Whiteboard: [devtools-html] leave-open → leave-open
Assignee: ejpbruel → nobody
This is quite an old bug with half-landed patches.  Let's mark this one done and carry over remaining work to somewhere new.
Assignee: nobody → ejpbruel
Closed: 3 years ago
Resolution: --- → FIXED
Summary: Change ThreadActor to protocol.js → Partially convert ThreadActor to protocol.js
Whiteboard: leave-open
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.