832470 - Watch expressions involving |this| sometimes showing a wrong result

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Girish Sharma [:Optimizer]]

Attachment: screenshot

See the screenshot. The method _showPopup has already been bound to InspectorPanel .

While in the watch expression this.searchBox and this.searchListBox both are undefined, they exist very well according to the expansion of |this| in the function scope block.

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: v1

This can happen in the content debugger as well. It's due to the extra function scope added on top. This was done to avoid avoid having the whole watch expressions array throw because of a single faulty expression, and also avoid weird stuff like // in an expression invalidating everything.

This was also the reason why |arguments| used to yield wrong results.

The extra function scope could be ditched altogether, but a few sanity checks need to be made beforehand, just to avoid syntax errors. Try..catch blocks could be moved inside an eval statement (to catch expressions like "throw new Error()").

Panos, let me know if this approach is sane. It seems to work no matter what I throw at clientEvaluate.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Which patch does this depend on? I can't get it to apply cleanly.

Comment 3

[Victor Porof [:vporof][:vp]]

(In reply to Panos Astithas [:past] from comment #2)
> Which patch does this depend on? I can't get it to apply cleanly.

I'll rebase.

Comment 4

[Victor Porof [:vporof][:vp]]

Attachment: v2

Applies cleanly on fx-team.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

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

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

I can't find a case where your code breaks, but I found one where this version causes SpiderMonkey to assert, whereas the previous didn't:

function(){}

This doesn't seem your fault though.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

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

Maybe Jim can think of some edge case.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Assertion with v2

That's the crash I got when I entered "function(){}" as a watch expression. The current code returns an error instead.

Comment 8

[Victor Porof [:vporof][:vp]]

(In reply to Panos Astithas [:past] from comment #5)
> Comment on attachment 704191 [details] [diff] [review]
> v1
> 
> Review of attachment 704191 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I can't find a case where your code breaks, but I found one where this
> version causes SpiderMonkey to assert, whereas the previous didn't:
> 
> function(){}
> 

I don't get the assertion in either case. Weird.
I'll add a couple of tests so we can land this at some point.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Victor Porof [:vp] from comment #8)
> I don't get the assertion in either case. Weird.

Huh, I don't get it either after updating to fx-team tip. The weirdest part is that I didn't get any JS engine changes when I updated.

Comment 10

[Victor Porof [:vporof][:vp]]

Attachment: v2.1

Added tests and a max-height property to #expressions. Resizing that container used to create some weird artifacts due to overflow.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 705816 [details] [diff] [review]
v2.1

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

Looks good to me.

::: browser/devtools/debugger/debugger-controller.js
@@ +1014,5 @@
> +      try {
> +        Reflect.parse(str);
> +        return str; // Watch expression can be executed safely.
> +      } catch (e) {
> +        return "\"" + e.name + ": " + e.message + "\""; // Syntax error.

Nit: I prefer to alternate between single and double quotes in these cases, but you might not care.

Comment 12

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/a9b475814d45

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/a9b475814d45

Comment 14

[Jim Blandy :jimb]

Comment on attachment 705816 [details] [diff] [review]
v2.1

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

The protocol really ought to provide a decent way to do this, without all the eval rigamarole. But as things stand, this seems like an okay workaround.

Comment 15

[Jim Blandy :jimb]

(Sorry, I didn't mean to clear past's 'feedback+' flag on that patch.)