Panos Astithas (he/him) [:past] (please ni?) Reporter
	Description • 13 years ago

The remote protocol specification describes the "substring" message that a Long String grip actor must respond to:

https://wiki.mozilla.org/Remote_Debugging_Protocol#Long_Strings

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 2 • 12 years ago

Removing this from depending on bug 781397 because that is hairier than I originally thought.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 3 • 12 years ago

Try: https://tbpl.mozilla.org/?tree=Try&rev=cb4fe4ff259a

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 4 • 12 years ago

xpcshell.ini got busted when I rebased on M-C >_<

New try push: https://tbpl.mozilla.org/?tree=Try&rev=e580dda8a8b5

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 5 • 12 years ago

…And for some reason my fix to the ini file didn't get pushed…

https://tbpl.mozilla.org/?tree=Try&rev=b1f1a8bd8d87

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 6 • 12 years ago

Comment on attachment 651020 [details] [diff] [review]
v1

Adding more to this patch, so removing the r? for now.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 7 • 12 years ago

Added more tests, and added support for long strings to the `ThreadClient` so that we can actually send long strings over the wire.

The remote debug protocol says a long string 'represents a very long string, where "very long" is defined at the server's discretion.' In this patch, I have made it so that every string whose length is equal to or larger than 200 chars is considered a long string. I am very open to changing how this works, but I'm not sure how I would change the test if we weren't considering some number N to be a cut off point where all strings longer than N were transferred as long strings automatically. Input in this area would be appreciated.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 8 • 12 years ago

Oh, and here is the new try push: https://tbpl.mozilla.org/?tree=Try&rev=e2d0691c2e58

	Rob Campbell [:rc] (:robcee)
	Comment 9 • 12 years ago

Comment on attachment 651020 [details] [diff] [review]
v1

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

couple of questions in line. I think I'm ok with this.

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +1045,5 @@
> + * A base actor for any actors that should only respond receive messages in the
> + * paused state. Subclasses must implement the isPaused method. You should never
> + * instantiate a PauseScopedActor directly, only through subclasses.
> + */
> +function PauseScopedActor()

need abstract and virtual void here. Just kiddin'!

@@ +1063,5 @@
> +      return aMethod.apply(this, arguments);
> +    } else {
> +      return this._wrongState();
> +    }
> +  };

Hmmm.

@@ +1089,5 @@
> + * @param aNewAttrs Object
> + *        The new attributes being set on the target.
> + */
> +function update(aTarget, aNewAttrs) {
> +  for (let key of Object.keys(aNewAttrs)) {

maybe I'm dumb this morning but how is this different from:

for (let name in aNewAttrs)

?

@@ +1110,4 @@
>    this.threadActor = aThreadActor;
>  }
>  
> +ObjectActor.prototype = Object.create(PauseScopedActor.prototype)

where's yer semi-colon? We don't go for any o' that newfangled "no semi-colons 'n' commas at the begginnin' o' lines" 'round here.

@@ +1116,4 @@
>    actorPrefix: "obj",
>  
> +  isPaused: function OA_isPaused() {
> +    return this.threadActor.state === "paused";

so WRONG_STATE_RESPONSE just goes away in this implementation?

@@ +1143,4 @@
>     * @param aRequest object
>     *        The protocol request object.
>     */
> +  onOwnPropertyNames: PauseScopedActor.withPaused(function OA_onOwnPropertyNames(aRequest) {

this and the following methods are getting rather long. Try to keep it under 80 characters like you're a hacker on a green screen terminal. You can break it up after the colon.

@@ +1155,4 @@
>     * @param aRequest object
>     *        The protocol request object.
>     */
> +  onPrototypeAndProperties: PauseScopedActor.withPaused(function OA_onPrototypeAndProperties(aRequest) {

ditto here.

@@ +1346,5 @@
> +
> +LongStringActor.prototype = Object.create(PauseScopedActor.prototype);
> +
> +update(LongStringActor.prototype, {
> +  actorPrefix: 'longString',

Panos was using double-quotes for these prefixes. Just sayin'.

@@ +1374,5 @@
> +  _substring: function LSA_substring(aIndex, aLength) {
> +    if (aIndex < 0) {
> +      throw this._indexError(aIndex);
> +    } else if (aIndex + aLength > this.stringLength) {
> +      throw this._indexError(aIndex + aLength);

do we need to throw here or could we just return the substr to the end of the string?

@@ +1435,5 @@
> +  "substring": LongStringActor.prototype.onSubstring
> +};
> +
> +
> +/**

ok.

::: toolkit/devtools/debugger/tests/unit/test_longstringactor.js
@@ +10,5 @@
> +
> +  // test_LSA_isPaused();    // no leak
> +  // test_LSA_disconnect();  // no leak
> +  // test_LSA_substring();   // no leak
> +  // test_LSA_grip();        // no leak

are we leaving these commented out?

	Rob Campbell [:rc] (:robcee)
	Comment 10 • 12 years ago

Comment on attachment 652009 [details] [diff] [review]
v2

with a successful try run and some cleanups from above.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 11 • 12 years ago

Updates:

- Changed to for (let name in aNewAttrs)
- Added missing semicolon
- Broke long lines to less than 80 chars
- Use double quotes for actor's prefix name
- Uncommented tests

Try:

https://tbpl.mozilla.org/?tree=Try&rev=160f33dcd5b5

Comments:

> so WRONG_STATE_RESPONSE just goes away in this implementation?

Yeah, it is replaced by the PauseScopedActor.prototype._wrongState method.

> do we need to throw here or could we just return the substr to the end of the
> string?

The reason I made it throw is because the client is asking for a string that is longer than we can provide, therefore we cannot fulfill the request. That sounds like something that should throw an error to me. I understand that this behavior deviates from the String.prototype.substr method, and I would be willing to change it if anyone feels strongly that the current behavior is bad.

> are we leaving these commented out?

Mistake left over from debugging >_<

	Jim Blandy :jimb
	Comment 12 • 12 years ago

(In reply to Nick Fitzgerald :fitzgen from comment #7)
> The remote debug protocol says a long string 'represents a very long string,
> where "very long" is defined at the server's discretion.' In this patch, I
> have made it so that every string whose length is equal to or larger than
> 200 chars is considered a long string. I am very open to changing how this
> works, but I'm not sure how I would change the test if we weren't
> considering some number N to be a cut off point where all strings longer
> than N were transferred as long strings automatically. Input in this area
> would be appreciated.

The only goal of the 'long string' thing is to keep the protocol from falling over whenever someone happens across some multi-dozen-megabyte string some crazy content has accumulated. Obviously, this kind of care needs to extend throughout the UI as well --- 
if the UI tries to get the entire string before displaying it, then we'll lose anyway. And the same general issue shows up in other contexts: can we safely inspect objects with millions of properties? So this bit of protocol is hardly a complete solution. But I at least didn't want the protocol to be part of the problem. :)

So the number should be larger than almost any string one is likely to come across, but small enough to protect the tools from locking up when we stumble across something big. 10000?

During testing and development, it might be good to set it at 5 or something, to exercise those code paths.

(It looks like the protocol has some bugs in this area, though; for example, the response to a prototypeAndProperties request requires that the property names be JSON strings, not grips. I should look through it and try to address those...)

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 13 • 12 years ago

Moved LONG_STRING_LENGTH to DebuggerServer.LONG_STRING_LENGTH and made it default to 10000.

In the unit test for long string grips, I change that value to 200 so that the mocked long string is long enough.

Regarding always having the LONG_STRING_LENGTH be very small for testing, I will defer to others.

	Jim Blandy :jimb
	Comment 14 • 12 years ago

Comment on attachment 652205 [details] [diff] [review]
v3

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

This looks pretty much like what's needed. Some opinions and suggestions attached.

::: toolkit/devtools/debugger/dbg-client.jsm
@@ +881,5 @@
> +   *        The pause-lifetime long string grip returned by the protocol.
> +   */
> +  pauseLongString: function TC_pauseLongString(aGrip) {
> +    if (!this._pauseLongStrings) {
> +      this._pauseLongStrings = {};

Do we need a separate table for long string actors? Actor names have to be globally unique --- otherwise the server can't tell who the message is directed to. I'd expect that simply registering this with _pauseGrips should be enough, and there's no need for _pauseLongStrings.

@@ +1039,5 @@
> +  get actor() { return this._grip.actor; },
> +
> +  _valid: true,
> +  get valid() { return this._valid; },
> +  set valid(aValid) { this._valid = !!aValid; },

I'm a 100% JS naif, and this is total style nit-picking, so just ignore this comment entirely. Here, I'll delete it before you ever read it. [clicks incorrectly; fails to delete comment; fails to notice that he has failed to delete comment]

I know GripClient does it, but is it really going to kill us to just let 'valid' be a simple property? I mean, *technically* I could store something in it that has an odd valueOf method, but... so don't do that, right?

Anyone trying to cause trouble could just redefine Object, and we'd lose our minds. Stuff like this getter/setter pair is like driving a stake into the ground and hoping the bad guy trips over it.

@@ +1058,5 @@
> +                   type: DebugProtocolTypes.substring,
> +                   start: aIndex,
> +                   length: aLength };
> +    this._client.request(packet, function (aResponse) {
> +      if (aCallback) {

Let's take out the 'if' here; it's not useful to call LongStringClient#substring with a missing callback.

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +918,5 @@
> +   * @param aString String
> +   *        The string we are checking the length of.
> +   */
> +  _stringIsLong: function TA__stringIsLong(aString) {
> +    return aString.length >= LONG_STRING_LENGTH;

Totally up to you, but personally, I'd just write out 'foo.length >= LONG_STRING_LENGTH' instead of providing a function. It makes it look like something complicated might be going on, when it's not.

@@ +1069,5 @@
>  
>  /**
> + * A base actor for any actors that should only respond receive messages in the
> + * paused state. Subclasses must implement the isPaused method. You should never
> + * instantiate a PauseScopedActor directly, only through subclasses.

This refactoring looks great!

Perhaps we could just require PauseScopedActors to have a (possibly null) threadActor property; then isPaused could be moved to PauseScopedActors, too.

I think it's easier on reviewers to post refactorings like this as a separate, prerequisite patch. That way, they can just ensure that it's a behavior-preserving patch, and not worry about whether there's new behavior being implemented.

@@ +1099,5 @@
> +   */
> +  _wrongState: function PSA_wrongState() {
> +    return {
> +      error: "wrongState",
> +      message: this.constructor.name +

Won't everyone get a this.constructor.name of "PauseScopedActor", instead of the most-derived class's name? You define subclasses with code like:

   SubclassConstructor.prototype = Object.create(PauseScopedActor.prototype)

and that replaces SubclassConstructor's JS-provided prototype object (which has a 'constructor' property set to Constructor) with a fresh object that does not, causing instances of SubclassConstructor to find PauseScopeActor.prototype's 'constructor' property.

@@ +1107,5 @@
> +};
> +
> +
> +/**
> + * Utility function for updating an object with the attributes of another

nit: Better to use the term "property" here. ECMAScript uses "attributes" for things like 'enumerable', 'writable', and so on: characteristics that a property might have.

@@ +1403,5 @@
> +  _substring: function LSA_substring(aIndex, aLength) {
> +    if (aIndex < 0) {
> +      throw this._indexError(aIndex);
> +    } else if (aIndex + aLength > this.stringLength) {
> +      throw this._indexError(aIndex + aLength);

It seems to me we should have our own error class (PacketError, say), instances of which have 'packet' property or method that produces an error packet. The catch clause already present in DebugServerConnection.onPacket would recognize PacketErrors, get the packet, and return it. Then we could use 'throw' and 'catch' in our packet handlers, for error conditions that could be reported back to the client, such as the one being reported here. The 'catch' clauses in LSA_grip and LSA_onSubString could then go away.

@@ +1428,5 @@
> +   *
> +   * @param aInitialLength Number
> +   *        The number of characters of this actor's string to send initially.
> +   */
> +  grip: function LSA_grip(aInitialLength) {

It seems to me the initial length should be a constant, like LONG_STRING_LENGTH. Perhaps it should be, in fact, LONG_STRING_LENGTH.

	Jim Blandy :jimb
	Comment 15 • 12 years ago

Comment on attachment 652205 [details] [diff] [review]
v3

(Didn't mean to un-obsolete this patch...)

	Jim Blandy :jimb
	Comment 16 • 12 years ago

Comment on attachment 652588 [details] [diff] [review]
v3.1

I submitted my comments on v3 after you posted v3.1, but if the only change is the long string limit size, then my feedback for v3 is still good for v3.1.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 17 • 12 years ago

(In reply to Jim Blandy :jimb from comment #14)
> Comment on attachment 652205 [details] [diff] [review]
> v3
> 
> Review of attachment 652205 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks pretty much like what's needed. Some opinions and suggestions
> attached.
> 
> ::: toolkit/devtools/debugger/dbg-client.jsm
> @@ +881,5 @@
> > +   *        The pause-lifetime long string grip returned by the protocol.
> > +   */
> > +  pauseLongString: function TC_pauseLongString(aGrip) {
> > +    if (!this._pauseLongStrings) {
> > +      this._pauseLongStrings = {};
> 
> Do we need a separate table for long string actors? Actor names have to be
> globally unique --- otherwise the server can't tell who the message is
> directed to. I'd expect that simply registering this with _pauseGrips should
> be enough, and there's no need for _pauseLongStrings.

Fine by me.

> 
> @@ +1039,5 @@
> > +  get actor() { return this._grip.actor; },
> > +
> > +  _valid: true,
> > +  get valid() { return this._valid; },
> > +  set valid(aValid) { this._valid = !!aValid; },
> 
> I'm a 100% JS naif, and this is total style nit-picking, so just ignore this
> comment entirely. Here, I'll delete it before you ever read it. [clicks
> incorrectly; fails to delete comment; fails to notice that he has failed to
> delete comment]
> 
> I know GripClient does it, but is it really going to kill us to just let
> 'valid' be a simple property? I mean, *technically* I could store something
> in it that has an odd valueOf method, but... so don't do that, right?
> 
> Anyone trying to cause trouble could just redefine Object, and we'd lose our
> minds. Stuff like this getter/setter pair is like driving a stake into the
> ground and hoping the bad guy trips over it.

Yeah I agree, I just didn't want to rock the boat since GripClient already does it. It seemed like it would be worse to have one be a simple property and the other have the get/set stuff. I can change both if we think that is for the best.

> 
> @@ +1058,5 @@
> > +                   type: DebugProtocolTypes.substring,
> > +                   start: aIndex,
> > +                   length: aLength };
> > +    this._client.request(packet, function (aResponse) {
> > +      if (aCallback) {
> 
> Let's take out the 'if' here; it's not useful to call
> LongStringClient#substring with a missing callback.

Agreed.

> 
> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +918,5 @@
> > +   * @param aString String
> > +   *        The string we are checking the length of.
> > +   */
> > +  _stringIsLong: function TA__stringIsLong(aString) {
> > +    return aString.length >= LONG_STRING_LENGTH;
> 
> Totally up to you, but personally, I'd just write out 'foo.length >=
> LONG_STRING_LENGTH' instead of providing a function. It makes it look like
> something complicated might be going on, when it's not.

I kind of like having this compartmentalized because if we ever decide we want to do something a little more complicated and start using some heuristics or something, it is really easy to do.

> 
> @@ +1069,5 @@
> >  
> >  /**
> > + * A base actor for any actors that should only respond receive messages in the
> > + * paused state. Subclasses must implement the isPaused method. You should never
> > + * instantiate a PauseScopedActor directly, only through subclasses.
> 
> This refactoring looks great!
> 
> Perhaps we could just require PauseScopedActors to have a (possibly null)
> threadActor property; then isPaused could be moved to PauseScopedActors, too.

Good idea!


> I think it's easier on reviewers to post refactorings like this as a
> separate, prerequisite patch. That way, they can just ensure that it's a
> behavior-preserving patch, and not worry about whether there's new behavior
> being implemented.

Will do, next time.

> 
> @@ +1099,5 @@
> > +   */
> > +  _wrongState: function PSA_wrongState() {
> > +    return {
> > +      error: "wrongState",
> > +      message: this.constructor.name +
> 
> Won't everyone get a this.constructor.name of "PauseScopedActor", instead of
> the most-derived class's name? You define subclasses with code like:
> 
>    SubclassConstructor.prototype = Object.create(PauseScopedActor.prototype)
> 
> and that replaces SubclassConstructor's JS-provided prototype object (which
> has a 'constructor' property set to Constructor) with a fresh object that
> does not, causing instances of SubclassConstructor to find
> PauseScopeActor.prototype's 'constructor' property.

Interesting, I had assumed (based on absolutely nothing other than intuition) that the constructor property was set on calling with "new". Sounds like I need to write a test!

> 
> @@ +1107,5 @@
> > +};
> > +
> > +
> > +/**
> > + * Utility function for updating an object with the attributes of another
> 
> nit: Better to use the term "property" here. ECMAScript uses "attributes"
> for things like 'enumerable', 'writable', and so on: characteristics that a
> property might have.

Ok.

> 
> @@ +1403,5 @@
> > +  _substring: function LSA_substring(aIndex, aLength) {
> > +    if (aIndex < 0) {
> > +      throw this._indexError(aIndex);
> > +    } else if (aIndex + aLength > this.stringLength) {
> > +      throw this._indexError(aIndex + aLength);
> 
> It seems to me we should have our own error class (PacketError, say),
> instances of which have 'packet' property or method that produces an error
> packet. The catch clause already present in DebugServerConnection.onPacket
> would recognize PacketErrors, get the packet, and return it. Then we could
> use 'throw' and 'catch' in our packet handlers, for error conditions that
> could be reported back to the client, such as the one being reported here.
> The 'catch' clauses in LSA_grip and LSA_onSubString could then go away.

I agree that this would be a positive change. Should I make this a separate patch?

> 
> @@ +1428,5 @@
> > +   *
> > +   * @param aInitialLength Number
> > +   *        The number of characters of this actor's string to send initially.
> > +   */
> > +  grip: function LSA_grip(aInitialLength) {
> 
> It seems to me the initial length should be a constant, like
> LONG_STRING_LENGTH. Perhaps it should be, in fact, LONG_STRING_LENGTH.

Sounds better than hard-coding 0 in the methods that call this one :)

Will post a new patch after lunch!

	Jim Blandy :jimb
	Comment 18 • 12 years ago

(In reply to Nick Fitzgerald :fitzgen from comment #17)
> Yeah I agree, I just didn't want to rock the boat since GripClient already
> does it. It seemed like it would be worse to have one be a simple property
> and the other have the get/set stuff. I can change both if we think that is
> for the best.

Go for it, I say. Let them tell us we're wrong. :D

> > Totally up to you, but personally, I'd just write out 'foo.length >=
> > LONG_STRING_LENGTH' instead of providing a function. It makes it look like
> > something complicated might be going on, when it's not.
> 
> I kind of like having this compartmentalized because if we ever decide we
> want to do something a little more complicated and start using some
> heuristics or something, it is really easy to do.

Okay.

> > @@ +1099,5 @@
> > > +   */
> > > +  _wrongState: function PSA_wrongState() {
> > > +    return {
> > > +      error: "wrongState",
> > > +      message: this.constructor.name +
> > 
> > Won't everyone get a this.constructor.name of "PauseScopedActor", instead of
> > the most-derived class's name? You define subclasses with code like:
> > 
> >    SubclassConstructor.prototype = Object.create(PauseScopedActor.prototype)
> > 
> > and that replaces SubclassConstructor's JS-provided prototype object (which
> > has a 'constructor' property set to Constructor) with a fresh object that
> > does not, causing instances of SubclassConstructor to find
> > PauseScopeActor.prototype's 'constructor' property.
> 
> Interesting, I had assumed (based on absolutely nothing other than
> intuition) that the constructor property was set on calling with "new".
> Sounds like I need to write a test!

Yeah. Every function has a .prototype property referring to an object that has a (non-enumerable) .constructor property. It's set up when the function object is constructed.

> > @@ +1403,5 @@
> > > +  _substring: function LSA_substring(aIndex, aLength) {
> > > +    if (aIndex < 0) {
> > > +      throw this._indexError(aIndex);
> > > +    } else if (aIndex + aLength > this.stringLength) {
> > > +      throw this._indexError(aIndex + aLength);
> > 
> > It seems to me we should have our own error class (PacketError, say),
> > instances of which have 'packet' property or method that produces an error
> > packet. The catch clause already present in DebugServerConnection.onPacket
> > would recognize PacketErrors, get the packet, and return it. Then we could
> > use 'throw' and 'catch' in our packet handlers, for error conditions that
> > could be reported back to the client, such as the one being reported here.
> > The 'catch' clauses in LSA_grip and LSA_onSubString could then go away.
> 
> I agree that this would be a positive change. Should I make this a separate
> patch?

Sure --- probably even a separate, follow-up bug.

	Dave Camp (:dcamp)
	Comment 19 • 12 years ago

(In reply to Jim Blandy :jimb from comment #18)
> (In reply to Nick Fitzgerald :fitzgen from comment #17)
> > Yeah I agree, I just didn't want to rock the boat since GripClient already
> > does it. It seemed like it would be worse to have one be a simple property
> > and the other have the get/set stuff. I can change both if we think that is
> > for the best.
> 
> Go for it, I say. Let them tell us we're wrong. :D

You are not wrong.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 20 • 12 years ago

- Put long string grips in the same object as pause grips
- Change "valid" to a normal property
- Removed check for the callback in LongStringGrip.prototype.substring
- Moved isPaused to PauseScopedActor.prototype
- Added test for PauseScopedActor.prototype._wrongState giving the right actor name, and fixed how it was failing that test
- s/attributes/properties/ in update's docs
- Added LONG_STRING_INITIAL_LENGTH

New try push: https://tbpl.mozilla.org/?tree=Try&rev=8a6f32d11db6

	Panos Astithas (he/him) [:past] (please ni?) Reporter
	Comment 21 • 12 years ago

Comment on attachment 652963 [details] [diff] [review]
v4

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

::: toolkit/devtools/debugger/dbg-client.jsm
@@ +879,5 @@
> +   *
> +   * @param aGrip Object
> +   *        The pause-lifetime long string grip returned by the protocol.
> +   */
> +  pauseLongString: function TC_pauseLongString(aGrip) {

Isn't the "pause" part superfluous?

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +1370,5 @@
> + *
> + * @param aString String
> + *        The string.
> + * @param aThreadActor ThreadActor
> + *        Optional. The parent thread actor for this long string. If a

This is optional for reusability purposes, as discussed in IRC a few weeks ago, right? I need to see if I can make this change to the other constructors as well.

@@ +1409,5 @@
> +  _substring: function LSA_substring(aIndex, aLength) {
> +    if (aIndex < 0) {
> +      throw this._indexError(aIndex);
> +    } else if (aIndex + aLength > this.stringLength) {
> +      throw this._indexError(aIndex + aLength);

Throwing here seems rather harsh to clients. Instinctively I'd expect the behavior of substr or strncpy. Fetching the whole string in a loop would be easier if the developer doesn't have to worry about getting the last chunk size right.

One way or another this needs to be documented in the protocol, so if Jim's fine with it, I'm OK too.

	Panos Astithas (he/him) [:past] (please ni?) Reporter
	Comment 22 • 12 years ago

And of course, THANK YOU for taking care of this!

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 23 • 12 years ago

(In reply to Panos Astithas [:past] from comment #21)
> Comment on attachment 652963 [details] [diff] [review]
> v4
> 
> Review of attachment 652963 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/debugger/dbg-client.jsm
> @@ +879,5 @@
> > +   *
> > +   * @param aGrip Object
> > +   *        The pause-lifetime long string grip returned by the protocol.
> > +   */
> > +  pauseLongString: function TC_pauseLongString(aGrip) {
> 
> Isn't the "pause" part superfluous?

Well not exactly, since (in this patch) the server supports both pause scoped and thread scoped long strings, but both can only be accessed while the thread is paused.

I can remove the thread scoped long strings if we think we will never use them.


> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +1370,5 @@
> > + *
> > + * @param aString String
> > + *        The string.
> > + * @param aThreadActor ThreadActor
> > + *        Optional. The parent thread actor for this long string. If a
> 
> This is optional for reusability purposes, as discussed in IRC a few weeks
> ago, right? I need to see if I can make this change to the other
> constructors as well.

Correct.

> @@ +1409,5 @@
> > +  _substring: function LSA_substring(aIndex, aLength) {
> > +    if (aIndex < 0) {
> > +      throw this._indexError(aIndex);
> > +    } else if (aIndex + aLength > this.stringLength) {
> > +      throw this._indexError(aIndex + aLength);
> 
> Throwing here seems rather harsh to clients. Instinctively I'd expect the
> behavior of substr or strncpy. Fetching the whole string in a loop would be
> easier if the developer doesn't have to worry about getting the last chunk
> size right.
> 
> One way or another this needs to be documented in the protocol, so if Jim's
> fine with it, I'm OK too.

I can change this since it seems no one expects this behavior from long strings.

(In reply to Panos Astithas [:past] from comment #22)
> And of course, THANK YOU for taking care of this!

No problem :)

	Panos Astithas (he/him) [:past] (please ni?) Reporter
	Comment 24 • 12 years ago

(In reply to Nick Fitzgerald :fitzgen from comment #23)
> (In reply to Panos Astithas [:past] from comment #21)
> > ::: toolkit/devtools/debugger/dbg-client.jsm
> > @@ +879,5 @@
> > > +   *
> > > +   * @param aGrip Object
> > > +   *        The pause-lifetime long string grip returned by the protocol.
> > > +   */
> > > +  pauseLongString: function TC_pauseLongString(aGrip) {
> > 
> > Isn't the "pause" part superfluous?
> 
> Well not exactly, since (in this patch) the server supports both pause
> scoped and thread scoped long strings, but both can only be accessed while
> the thread is paused.
> 
> I can remove the thread scoped long strings if we think we will never use
> them.

My point was that since long strings can only be accessed when paused, using the prefix didn't convey much useful information. Unless the prefix refers to the actor lifetime and you expect to have a threadLongString method in the future.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 25 • 12 years ago

Try: 
https://tbpl.mozilla.org/?tree=Try&rev=4767a05b5bf9

Changes:
- Removed thread scoped long strings and renamed methods related to long string grips to "longString*" instead of "pauseLongString*"
- Changed behavior of calling substring on a long string to exactly mimic String.prototype.substr

	Jim Blandy :jimb
	Comment 26 • 12 years ago

Since strings are constants, there's no way the answer to a request to a long string actor can be affected by the state of the debuggee. As long as we know that the actor is actually alive --- whether pause-lifetime or thread-lifetime --- I don't think there's any reason to impose further constraints on when long string actors can be communicated with.

(In reply to Panos Astithas [:past] from comment #21)
> Throwing here seems rather harsh to clients. Instinctively I'd expect the
> behavior of substr or strncpy. Fetching the whole string in a loop would be
> easier if the developer doesn't have to worry about getting the last chunk
> size right.

Yeah, I agree --- if the client can simply ask for whatever size chunk they like and receive as much as there is, that seems like the easiest API to use.

> One way or another this needs to be documented in the protocol, so if Jim's
> fine with it, I'm OK too.

Here's possible documentation for this change:
https://github.com/jimblandy/DebuggerDocs/compare/clip-long-strings

	Jim Blandy :jimb
	Comment 27 • 12 years ago

Comment on attachment 653940 [details] [diff] [review]
v5

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

This is looking good, just a few comments:

::: toolkit/devtools/debugger/dbg-client.jsm
@@ +885,5 @@
> +  _ensurePauseGrips: function TC_ensurePauseGrips() {
> +    if (!this._pauseGrips) {
> +      this._pauseGrips = {};
> +    }
> +  },

It doesn't seem like there's any reason to allocate the pause grips table lazily. I suggest:

- Deleting this function entirely, and dropping all calls to it.

- Setting _pauseGrips to {} in the constructor and in _clearPauseGrips.

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +869,5 @@
> +   *        The string we are creating a grip for.
> +   */
> +  longStringGrip: function TA_longStringGrip(aString, aPool) {
> +    if (!this._pausePool) {
> +      throw "LongString grip requested while not paused.";

Is there a reason not to throw an instance of Error here? That way we get a backtrace.

@@ +887,5 @@
> +    return actor.grip();
> +  },
> +
> +  /**
> +   * Returns true if the string is sufficiently long enough to use a

This sounds redundant: "sufficiently long" or "long enough"?

@@ +1048,5 @@
> + * paused state. Subclasses may expose a `threadActor` which is used to help
> + * determine when we are in a paused state. Subclasses should set their own
> + * "constructor" property if they want better error messages. You should never
> + * instantiate a PauseScopedAc
> +tor directly, only through subclasses.

Accidental line break?

@@ +1383,5 @@
> +      return { "type": "longString",
> +               "initial": this.string.substr(0, DebuggerServer.LONG_STRING_INITIAL_LENGTH),
> +               "length": this.stringLength,
> +               "actor": this.actorID };
> +    } catch (error) {

Do we still need this 'catch' clause? The only thing I can see that might fail is the 'substr', and that'll only throw if you apply it to something with a toString method that throws... but this.string is always supposed to be a string.

@@ +1398,5 @@
> +  onSubstring: PauseScopedActor.withPaused(function LSA_onSubString(aRequest) {
> +    try {
> +      return { "from": this.actorID,
> +               "substring": this.string.substr(aRequest.start, aRequest.length) };
> +    } catch (error) {

Can't this 'catch' go, too?

::: toolkit/devtools/debugger/server/dbg-server.js
@@ +61,5 @@
>    xpcInspector: null,
>    _allowConnection: null,
>  
> +  LONG_STRING_LENGTH: 10000,
> +  LONG_STRING_INITIAL_LENGTH: 0,

Is this the value we want this to have in production? I'd expect at least 1000...

	Panos Astithas (he/him) [:past] (please ni?) Reporter
	Comment 28 • 12 years ago

(In reply to Jim Blandy :jimb from comment #27)
> ::: toolkit/devtools/debugger/server/dbg-server.js
> @@ +61,5 @@
> >    xpcInspector: null,
> >    _allowConnection: null,
> >  
> > +  LONG_STRING_LENGTH: 10000,
> > +  LONG_STRING_INITIAL_LENGTH: 0,
> 
> Is this the value we want this to have in production? I'd expect at least
> 1000...

Yes, I missed that. I had mentioned in another bug that when transferring scripts we could just use an initial length of zero as a micro-optimization (since we would always want to have all of the script available when it's time to display it), but this is a pessimization for the general case where the frontend might display whatever is already received, plus an ellipsis.

	Panos Astithas (he/him) [:past] (please ni?) Reporter
	Comment 29 • 12 years ago

Comment on attachment 653940 [details] [diff] [review]
v5

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

I can't find anything more to add to Jim's valid points.

::: toolkit/devtools/debugger/dbg-client.jsm
@@ +1047,5 @@
> +                   type: DebugProtocolTypes.substring,
> +                   start: aIndex,
> +                   length: aLength };
> +    this._client.request(packet, function (aResponse) {
> +      // TODO fitzgen: error handling here

If you won't do this here, then perhaps file a followup bug and put it in the comment?

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 30 • 12 years ago

(In reply to Jim Blandy :jimb from comment #26)
> Since strings are constants, there's no way the answer to a request to a
> long string actor can be affected by the state of the debuggee. As long as
> we know that the actor is actually alive --- whether pause-lifetime or
> thread-lifetime --- I don't think there's any reason to impose further
> constraints on when long string actors can be communicated with.

So essentially, LongStringActor should no longer subclass PauseScopedActor. Ok. I'm going to leave the PauseScopedActor refactors with regards to ObjectActor in there anyways because I think they were a positive change either way.

> 
> (In reply to Panos Astithas [:past] from comment #21)
> > Throwing here seems rather harsh to clients. Instinctively I'd expect the
> > behavior of substr or strncpy. Fetching the whole string in a loop would be
> > easier if the developer doesn't have to worry about getting the last chunk
> > size right.
> 
> Yeah, I agree --- if the client can simply ask for whatever size chunk they
> like and receive as much as there is, that seems like the easiest API to use.

That behavior is gone now, it just uses `String.prototype.substr`.

> 
> > One way or another this needs to be documented in the protocol, so if Jim's
> > fine with it, I'm OK too.
> 
> Here's possible documentation for this change:
> https://github.com/jimblandy/DebuggerDocs/compare/clip-long-strings

Looks good to me.

(In reply to Panos Astithas [:past] from comment #29)
> Comment on attachment 653940 [details] [diff] [review]
> v5
> 
> Review of attachment 653940 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I can't find anything more to add to Jim's valid points.
> 
> ::: toolkit/devtools/debugger/dbg-client.jsm
> @@ +1047,5 @@
> > +                   type: DebugProtocolTypes.substring,
> > +                   start: aIndex,
> > +                   length: aLength };
> > +    this._client.request(packet, function (aResponse) {
> > +      // TODO fitzgen: error handling here
> 
> If you won't do this here, then perhaps file a followup bug and put it in
> the comment?

I can actually get rid of that altogether. At the time of writing this code, I didn't realize that the client does error checking itself before passing on the response and if there is an error, it just doesn't give anything. I will add another test case for the error case though.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 31 • 12 years ago

Try:
https://tbpl.mozilla.org/?tree=Try&rev=ee798b302139

Changes:
- LongStringActor no longer inherits from PauseScopedActor, and so can be accessed even when the thread is running. As Jim explained, this is ok b/c strings are immutable.
- ThreadClient.prototype._ensurePauseGrips is gone, ThreadClient.prototype._pauseGrips is set in the constructor and in ThreadClient.prototype._clearPauseGrips
- Throw an error instead of a string in ThreadActor.prototype.longStringGrip
- s/sufficiently long enough/long enough/
- Remove accidental line break
- Remove now-unnecessary catch statements in LongStringActor.prototype methods (good /catch/ Jim!) /fitzgen hides for making such a bad pun
- Changed LONG_STRING_INITIAL_LENGTH to 1000
- Added a test case for LongStringClient.prototype.substring's error case

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 33 • 12 years ago

Try:
https://tbpl.mozilla.org/?tree=Try&rev=2feba01eb88e

Changes:
- Added a length accessor to LongStringClient
- Made the LongStringClient.prototype.substring callback take the actual response instead of response.substring so that the consumer can do proper error handling

	Jim Blandy :jimb
	Comment 34 • 12 years ago

Comment on attachment 654411 [details] [diff] [review]
v7

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

This looks great --- but I really want the length->end change made (see the in-line comments) before we land it.

::: toolkit/devtools/debugger/dbg-client.jsm
@@ +1037,5 @@
> +                   start: aIndex,
> +                   length: aLength };
> +    this._client.request(packet, function (aResponse) {
> +      aCallback(aResponse);
> +    });

You can replace:
function (aResponse) { aCallback(aResponse); }
with simply:
aCallback

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +1384,5 @@
> +   *        The protocol request object.
> +   */
> +  onSubstring: function LSA_onSubString(aRequest) {
> +    return { "from": this.actorID,
> +             "substring": this.string.substr(aRequest.start, aRequest.length) };

String.prototype.substr takes (start, end) arguments, not (start, length) arguments. This'll work as long as the start position is zero. The tests only pass because you've written them as if they were passing (start, end) too.

I think the protocol should change to match the ECMAScript function: the 'length' field of the long string grip actor "substring" request should become 'end', and the values should both be clipped to fall between 0 and the length. In other words, everything continues to mean what it does as you've written the code; just the name of the "substring" request property changes.

	Jim Blandy :jimb
	Comment 35 • 12 years ago

I've updated the wiki docs for the long string grip actor 'substring' request.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 36 • 12 years ago

(In reply to Jim Blandy :jimb from comment #34)
> Comment on attachment 654411 [details] [diff] [review]
> v7
> 
> Review of attachment 654411 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks great --- but I really want the length->end change made (see the
> in-line comments) before we land it.
> 
> ::: toolkit/devtools/debugger/dbg-client.jsm
> @@ +1037,5 @@
> > +                   start: aIndex,
> > +                   length: aLength };
> > +    this._client.request(packet, function (aResponse) {
> > +      aCallback(aResponse);
> > +    });
> 
> You can replace:
> function (aResponse) { aCallback(aResponse); }
> with simply:
> aCallback

Good catch, I should have changed that after removing the test for if aCallback existed.

> 
> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +1384,5 @@
> > +   *        The protocol request object.
> > +   */
> > +  onSubstring: function LSA_onSubString(aRequest) {
> > +    return { "from": this.actorID,
> > +             "substring": this.string.substr(aRequest.start, aRequest.length) };
> 
> String.prototype.substr takes (start, end) arguments, not (start, length)
> arguments. This'll work as long as the start position is zero. The tests
> only pass because you've written them as if they were passing (start, end)
> too.
> 
> I think the protocol should change to match the ECMAScript function: the
> 'length' field of the long string grip actor "substring" request should
> become 'end', and the values should both be clipped to fall between 0 and
> the length. In other words, everything continues to mean what it does as
> you've written the code; just the name of the "substring" request property
> changes.

I'm pretty sure you are thinking of `String.prototype.slice`, which does take (start, end), but substr takes (start, length). If you think (start, end) is the right way to go, I can just change over to using `.slice` instead of `.substr`.

https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/String/substr
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/String/slice

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 37 • 12 years ago

Pass aCallback directly in `this._client.request` as per Jim's feedback.

Will wait to ask for checkin until we resolve whether we want to mimic `String.prototype.substr` or `String.prototype.slice`.

	Jim Blandy :jimb
	Comment 38 • 12 years ago

(In reply to Nick Fitzgerald :fitzgen from comment #36)
> (In reply to Jim Blandy :jimb from comment #34)
> > I think the protocol should change to match the ECMAScript function: the
> > 'length' field of the long string grip actor "substring" request should
> > become 'end', and the values should both be clipped to fall between 0 and
> > the length. In other words, everything continues to mean what it does as
> > you've written the code; just the name of the "substring" request property
> > changes.
> 
> I'm pretty sure you are thinking of `String.prototype.slice`, which does
> take (start, end), but substr takes (start, length). If you think (start,
> end) is the right way to go, I can just change over to using `.slice`
> instead of `.substr`.

Wow, what a mess. I looked up 'substr' in the ECMAScript standard, found 'substring' instead, and never noticed that they were different functions.

It turns out that 'substr' is not in the ECMAScript standard; it was present in JS 1.0, but didn't make it into the standard.

The methods ECMAScript does specify, slice and substring, are both (start, end) functions. That seems to be the cool thing, so let's go with that. (And let's use 'substring' in the code, too, to avoid confusion.)

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 39 • 12 years ago

Try:
https://tbpl.mozilla.org/?tree=Try&rev=232258ce274e

Changes:
- Using (start, end) instead of (start, length)

	Panos Astithas (he/him) [:past] (please ni?) Reporter
	Comment 40 • 12 years ago

Comment on attachment 655089 [details] [diff] [review]
v8

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

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +883,5 @@
> +   *
> +   * @param aString String
> +   *        The string we are creating a grip for.
> +   */
> +  longStringGrip: function TA_longStringGrip(aString, aPool) {

aPool looks like leftover from a previous refactoring.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 41 • 12 years ago

Removed unused aPool argument.

	Jim Blandy :jimb
	Comment 42 • 12 years ago

Comment on attachment 655639 [details] [diff] [review]
v8.1

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

Looks great!

	Jim Blandy :jimb
	Comment 43 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a9c7ecef7bd

	Ryan VanderMeulen [:RyanVM]
	Comment 44 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/1a9c7ecef7bd