[jsdbg2] Expose Debugger.Source.prototype.canonicalId

RESOLVED FIXED in Firefox 44

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

unspecified
mozilla44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment)

The Debugger API is designed such that Debugger objects are not shared across actor, and each actor gets and uses its own Debugger object.

However, we want to link to and from sources between tools, implement source mapping once for all tools, etc and this leads to having one canonical store of sources on each of our "debuggee-target" actors (TabActor, RootActor, AddonActor, WorkerActor). Each child actor of these "debuggee-target" actors would like to share the same set of sources with the other children, and the sources are stored in the "debuggee-target" actor.

But because each child actor is using its own Debugger, Debugger.Source instances which represent the *same* source (ie, are backed by the same js::ScriptSourceObject) are not === to one another.

I propose implementing Debugger.Source.prototype.canonicalId which gives out an id suitable for === comparison and hashing/keying but not ordering.

This would just be the underlying ScriptSource pointer (since ScriptSource is never relocated, unlike ScriptSourceObject).
I remember a use case now: caching sourcemaps. We could usually use the sourceMapURL, but when pretty-printing we don't have that (we automatically generate it). We actually fixed that by adding the ability for forcefully set a sourceMapURL on a source such that all `Debugger.Source` instances see it.

Since we fixed it that way, I don't actually have a compelling use case right now. We can hold off on this until we integrate sourcemaps everywhere (very soon) and generally integrate debugger functionality better in other tools.

I would like to discuss the benefits of using multiple Debugger objects; it introduces complexity and I don't see any huge advantages if we assume the debugger exists (which will always be true after bug 1132501).
Created attachment 8672001 [details] [diff] [review]
Add Debugger.Source.prototype.canonicalId
Attachment #8672001 - Flags: review?(ejpbruel)
Eddy, if you don't feel comfortable reviewing Debugger patches anymore, then I can find someone else.

Note that while splitting out the js::Value::isNumberRepresentable change isn't strictly necessary yet, but it will be in some of my upcoming patches so I got it out of the way now.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=544b5a647441
Blocks: 1213436
Comment on attachment 8672001 [details] [diff] [review]
Add Debugger.Source.prototype.canonicalId

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

This looks fine to me. The only thing I'd like to ask is that you add a test to make sure this works between Debuggers in two different workers as well. r+ with that.

::: js/public/Value.h
@@ +1014,5 @@
> +     * Returns false if creating a NumberValue containing the given type would
> +     * be lossy, true otherwise.
> +     */
> +    template <typename T>
> +    static bool isNumberRepresentable(const T t) {

What if t == NaN? NaN is certainly representable as a number, yet T(double(NaN)) != NaN. Is this an edge case we need to consider?

@@ -1610,5 @@
>  template <typename T>
>  static inline Value
>  NumberValue(const T t)
>  {
> -    MOZ_ASSERT(T(double(t)) == t, "value creation would be lossy");

Looks like we were already using this code, so I guess my comment above can be ignored.

::: js/src/doc/Debugger/Debugger.Source.md
@@ +40,5 @@
>  A `Debugger.Source` instance inherits the following accessor properties
>  from its prototype:
>  
> +`canonicalId`
> +:   A stable, unique identifier for the source referent. This identifier is

If this is a globally unique identifier, why don't we simply call it a guid? This term is better known than canonicalId as far as I know.

That said, 'canonicalId' is not ambiguous, so I'll leave it up to you.

@@ +42,5 @@
>  
> +`canonicalId`
> +:   A stable, unique identifier for the source referent. This identifier is
> +    suitable for checking if two `Debugger.Source` instances originating from
> +    different `Debugger` instances refer to the same source. It is not suitable

How is equivalence for sources defined? Are they equivalent if they have the same url? Different urls but the same source text? What about sources without a url?

I think it would be beneficial to add a comment here that clarifies this.

::: js/src/jit-test/tests/debug/Source-canonicalId.js
@@ +1,3 @@
> +// Test Debugger.Source.prototype.canonicalId
> +
> +const g = newGlobal();

Thinking ahead here a little bit: we eventually want to implement a global (i.e. shared between threads) breakpoint store for the debugger. One consequence of this is that setting a breakpoint on a D.S. instance in one worker should cause the same breakpoint to be set on all D.S. instances in other workers representing the same source.

This API seems perfectly suitable to solve that problem, but given that we want to use it for this purpose eventually, could you do me a favor and add a test to make sure that this works with two different workers?
Attachment #8672001 - Flags: review?(ejpbruel) → review+
(In reply to Eddy Bruel [:ejpbruel] from comment #4)
> Comment on attachment 8672001 [details] [diff] [review]
> Add Debugger.Source.prototype.canonicalId
> 
> Review of attachment 8672001 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine to me. The only thing I'd like to ask is that you add a test
> to make sure this works between Debuggers in two different workers as well.
> r+ with that.

Unfortunately, JSRuntimes do not share ScriptSource instances, and without that it is impossible to have the canonicalId be the same across workers. I will note this in the docs.

If/when bug 1211723 is implemented, then we can revisit.

> If this is a globally unique identifier, why don't we simply call it a guid?
> This term is better known than canonicalId as far as I know.
> 
> That said, 'canonicalId' is not ambiguous, so I'll leave it up to you.

I think "guid" is usually interpreted as a synonym for "uuid", which has a pretty specific meaning and form that is not what we are implementing here. I am going to leave it as canonicalId.

> How is equivalence for sources defined? Are they equivalent if they have the
> same url? Different urls but the same source text? What about sources
> without a url?
> 
> I think it would be beneficial to add a comment here that clarifies this.

Will do.

> ::: js/src/jit-test/tests/debug/Source-canonicalId.js
> @@ +1,3 @@
> > +// Test Debugger.Source.prototype.canonicalId
> > +
> > +const g = newGlobal();
> 
> Thinking ahead here a little bit: we eventually want to implement a global
> (i.e. shared between threads) breakpoint store for the debugger. One
> consequence of this is that setting a breakpoint on a D.S. instance in one
> worker should cause the same breakpoint to be set on all D.S. instances in
> other workers representing the same source.
> 
> This API seems perfectly suitable to solve that problem, but given that we
> want to use it for this purpose eventually, could you do me a favor and add
> a test to make sure that this works with two different workers?

Unfortunately, because (as I mentioned above) ScriptSources aren't shared across JSRuntimes, this API is not suitable for that. The only way would be to do full character by character comparison.

How often is the same script loaded in multiple workers? I'd imagine that this doesn't happen very often in practice, but I have no data.

Personally, I think we are probably fine with a different breakpoint store per thread, or using our existing "persistent id" heuristic that is based on URL/sourceMapURL/displayURL.
See Also: → bug 1211723
https://hg.mozilla.org/mozilla-central/rev/80ce4acb7a43
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.