| Summary: | Debugger has unexpected effect on program correctness | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||||||||
| Component: | WebCore JavaScript | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | buildbot, commit-queue, fpizlo, joepeck, keith_miller, mark.lam, msaboff, rniwa, saam | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=172684 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
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. 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
Even if we move to a builtin, don't we need to remove the for...of? (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. Created attachment 312694 [details]
[PATCH] Proposed Fix
Comment on attachment 312694 [details]
[PATCH] Proposed Fix
r=me
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 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 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 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 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 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
Created attachment 312789 [details]
[PATCH] For Landing
Comment on attachment 312789 [details] [PATCH] For Landing Clearing flags on attachment: 312789 Committed r218223: <http://trac.webkit.org/changeset/218223> |
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.