Bug 172683 - Debugger has unexpected effect on program correctness
Summary: Debugger has unexpected effect on program correctness
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-27 13:46 PDT by Sam Weinig
Modified: 2017-06-13 17:01 PDT (History)
9 users (show)

See Also:


Attachments
Test case (936 bytes, text/html)
2017-05-27 13:46 PDT, Sam Weinig
no flags Details
[PATCH] Proposed Fix (7.14 KB, patch)
2017-06-12 14:06 PDT, Joseph Pecoraro
sbarati: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.26 MB, application/zip)
2017-06-12 15:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-elcapitan (1000.32 KB, application/zip)
2017-06-12 15:06 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-elcapitan (1.84 MB, application/zip)
2017-06-12 15:23 PDT, Build Bot
no flags Details
[PATCH] For Landing (7.15 KB, patch)
2017-06-13 11:36 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2017-05-27 13:46:54 PDT
Created attachment 311423 [details]
Test case

I was debugging an issue today and noticed I was getting different results depending on whether I had a breakpoint in the inspector. I reduced it down to a relative small test case attached.
Comment 1 Sam Weinig 2017-05-27 13:49:36 PDT
The test case is replacing the @@iterator on Array to a function that counts the number of times it is accessed, and then forwards on to the original iterator. 

So it seems, when debugging, something is accessing the Array.prototype[@@iterator] in an observable way, which is unexpected to me, though I don't know if it is a bug.
Comment 2 Joseph Pecoraro 2017-05-30 12:01:58 PDT
I think its reasonable to assume that having Web Inspector open should have as little impact on the page as possible, to avoid altering execution behavior of programs.

Adding a console.trace() to the Array.prototype[@@iterator] accessor we see:

    [Log] Trace
    	get (test-case.html:15)
    	_appendPropertyPreviews
    	_generatePreview
    	RemoteObject
    	_wrapObject
    	CallFrameProxy
    	wrapCallFrames
    	Global Code (test-case.html:21)

    [Log] Trace
    	get (test-case.html:15)
    	Set
    	_isPreviewableObject
    	_appendPropertyPreviews
    	_generatePreview
    	RemoteObject
    	_wrapObject
    	CallFrameProxy
    	wrapCallFrames
    	Global Code (test-case.html:21)

So this is Web Inspector using `for..of` inside the page with the InjectedScriptSource. This happens when generating information in the debugger, for data in the Scope Chain details sidebar and Object previews.

In order to avoid this we would need to eliminate observable operations (like for..of) in InjectedScriptSource. A big step in that direction (eliminating observable behavior) is moving it to be a built-in:
<https://webkit.org/b/152294> Web Inspector: Parse InjectedScriptSource as a built-in to get guaranteed non-user-overriden built-ins
Comment 3 Saam Barati 2017-05-31 15:03:35 PDT
Even if we move to a builtin, don't we need to remove the for...of?
Comment 4 Joseph Pecoraro 2017-06-01 14:00:18 PDT
(In reply to Saam Barati from comment #3)
> Even if we move to a builtin, don't we need to remove the for...of?

Yes we would need to move off of for..of to eliminate observable behavior.
Comment 5 Joseph Pecoraro 2017-06-12 14:06:05 PDT
Created attachment 312694 [details]
[PATCH] Proposed Fix
Comment 6 Saam Barati 2017-06-12 14:25:31 PDT
Comment on attachment 312694 [details]
[PATCH] Proposed Fix

r=me
Comment 7 Build Bot 2017-06-12 15:02:02 PDT
Comment on attachment 312694 [details]
[PATCH] Proposed Fix

Attachment 312694 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3919280

New failing tests:
inspector/injected-script/observable.html
Comment 8 Build Bot 2017-06-12 15:02:03 PDT
Created attachment 312702 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 9 Build Bot 2017-06-12 15:06:54 PDT
Comment on attachment 312694 [details]
[PATCH] Proposed Fix

Attachment 312694 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3919303

New failing tests:
inspector/injected-script/observable.html
Comment 10 Build Bot 2017-06-12 15:06:55 PDT
Created attachment 312703 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 11 Build Bot 2017-06-12 15:23:46 PDT
Comment on attachment 312694 [details]
[PATCH] Proposed Fix

Attachment 312694 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3919324

New failing tests:
inspector/injected-script/observable.html
Comment 12 Build Bot 2017-06-12 15:23:48 PDT
Created attachment 312709 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 13 Joseph Pecoraro 2017-06-13 11:36:17 PDT
Created attachment 312789 [details]
[PATCH] For Landing
Comment 14 WebKit Commit Bot 2017-06-13 16:57:59 PDT
Comment on attachment 312789 [details]
[PATCH] For Landing

Clearing flags on attachment: 312789

Committed r218223: <http://trac.webkit.org/changeset/218223>