Bug 172683 – Debugger has unexpected effect on program correctness

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Sam Weinig]

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.

[Sam Weinig]

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.

[Joseph Pecoraro]

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

[Saam Barati]

Even if we move to a builtin, don't we need to remove the for...of?

[Joseph Pecoraro]

(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.

[Joseph Pecoraro]

Created attachment 312694 [details]
[PATCH] Proposed Fix

[Saam Barati]

Comment on attachment 312694 [details]
[PATCH] Proposed Fix

r=me

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Joseph Pecoraro]

Created attachment 312789 [details]
[PATCH] For Landing

[WebKit Commit Bot]

Comment on attachment 312789 [details]
[PATCH] For Landing

Clearing flags on attachment: 312789

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