Bug 138839

Summary: Web Inspector: Debugger should not mutate variable when hovering mouse over ++n expression
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, graouts, joepeck, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Proposed Fix
timothy: review+, timothy: commit-queue-
[PATCH] Proposed Fix none

Description Joseph Pecoraro 2014-11-18 11:32:54 PST
* SUMMARY
When debugging javascript paused in the debugger, hovering over a line of the form ++n actually mutates the value of n. n++ does not have this behavior.

* TEST
<script type="text/javascript">
function test() {
    debugger;
    var n = 0;
    console.log(n);
    // Hover over next line. n shouldn't change
    n++;
    console.log(n);
    // Hover over next line. n changes but shouldn't
    ++n;
    console.log(n);
}
document.onload = test();
</script>

* STEPS TO REPRODUCE
1. Inspect test case
2. Reload or type test() into the console
3. Test hovering over n++ and see it doesn't mutate
4. Test hovering over ++n and see that it mutates n every time you hover over it
Comment 1 Radar WebKit Bug Importer 2014-11-18 11:33:10 PST
<rdar://problem/19018280>
Comment 2 Joseph Pecoraro 2014-11-18 11:36:57 PST
Created attachment 241796 [details]
[PATCH] Proposed Fix
Comment 3 Joseph Pecoraro 2014-11-18 11:37:53 PST
<rdar://problem/18995374>
Comment 4 Timothy Hatcher 2014-11-18 11:51:02 PST
Comment on attachment 241796 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=241796&action=review

> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorTokenTrackingController.js:490
> +            if (isExpression && token.string === "++")

What about --?
Comment 5 Joseph Pecoraro 2014-11-18 12:03:50 PST
(In reply to comment #4)
> Comment on attachment 241796 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241796&action=review
> 
> > Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorTokenTrackingController.js:490
> > +            if (isExpression && token.string === "++")
> 
> What about --?

Doh! Tim also caught other operators, like:

    n+=n;

I'll put up another patch.
Comment 6 Joseph Pecoraro 2014-11-18 14:01:06 PST
Created attachment 241810 [details]
[PATCH] Proposed Fix
Comment 7 Timothy Hatcher 2014-11-19 09:54:02 PST
Comment on attachment 241810 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=241810&action=review

> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorTokenTrackingController.js:492
> +            // Disallow operators. We want the hovered expression to be just a single operand.
> +            // Also, some operators can modify values, such as pre-increment and assignment operators.
> +            if (isExpression && token.type.contains("operator"))
> +                break;

Nice!
Comment 8 WebKit Commit Bot 2014-11-19 10:34:15 PST
Comment on attachment 241810 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 241810

Committed r176325: <http://trac.webkit.org/changeset/176325>
Comment 9 WebKit Commit Bot 2014-11-19 10:34:18 PST
All reviewed patches have been landed.  Closing bug.