Watch expressions involving |this| sometimes showing a wrong result

RESOLVED FIXED in Firefox 21

Status

defect
P2
normal
RESOLVED FIXED
7 years ago
Last year

People

(Reporter: Optimizer, Assigned: vporof)

Tracking

unspecified
Firefox 21
x86_64
Windows 7
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Posted image 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.
Posted patch v1 (obsolete) — Splinter Review
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.
Attachment #704191 - Flags: feedback?(past)
Priority: -- → P2
Summary: Watch expression in browser debugger showing wrong result. → Watch expressions involving |this| sometimes showing a wrong result
Which patch does this depend on? I can't get it to apply cleanly.
(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.
Posted patch v2Splinter Review
Applies cleanly on fx-team.
Attachment #704191 - Attachment is obsolete: true
Attachment #704191 - Flags: feedback?(past)
Attachment #704498 - Flags: feedback?(past)
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.
Attachment #704191 - Attachment is obsolete: false
Attachment #704191 - Attachment is obsolete: true
Comment on attachment 704498 [details] [diff] [review]
v2

Maybe Jim can think of some edge case.
Attachment #704498 - Flags: feedback?(past)
Attachment #704498 - Flags: feedback?(jimb)
Attachment #704498 - Flags: feedback+
Posted file Assertion with v2
That's the crash I got when I entered "function(){}" as a watch expression. The current code returns an error instead.
(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.
(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.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Posted patch v2.1Splinter Review
Added tests and a max-height property to #expressions. Resizing that container used to create some weird artifacts due to overflow.
Attachment #705816 - Flags: review?(past)
Blocks: 828987
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.
Attachment #705816 - Flags: review?(past) → review+
https://hg.mozilla.org/mozilla-central/rev/a9b475814d45
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21
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.
Attachment #705816 - Flags: feedback+
Attachment #704498 - Flags: feedback+
Attachment #704498 - Flags: feedback?(jimb)
(Sorry, I didn't mean to clear past's 'feedback+' flag on that patch.)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.