Summary: | Web Inspector: The JS code injected by worker inspector shouldn't be evaluated through JSMainThreadExecState | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Peter Wang <PeterHWang> | ||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | adamk, apavlov, bweinstein, ggaren, joepeck, keishi, loislo, mark.lam, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 93519 | ||||||||||||
Attachments: |
|
Description
Peter Wang
2012-08-29 06:30:12 PDT
Created attachment 161213 [details]
Patch
Looks fine to me, but I'm not a reviewer, so someone from the JSC side (e.g., Sam) will be the right person to r+. Comment on attachment 161213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161213&action=review > Source/WebCore/ChangeLog:10 > + No new test case. Can this be tested? > Source/WebCore/bindings/js/ScriptFunctionCall.cpp:40 > +#ifndef NDEBUG Do we have debug-related behavior here? > Source/WebCore/inspector/InjectedScriptManager.h:71 > + explicit InjectedScriptManager(InspectedStateAccessCheck, bool = false); bool isForWorker = false (In reply to comment #3) > (From update of attachment 161213 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161213&action=review > > > Source/WebCore/ChangeLog:10 > > + No new test case. > > Can this be tested? Taking QT debug build as example, without my patch, when we click the link on the worker side-panel(refer to the attached picture), the Browser hits the assert statement. > > Source/WebCore/bindings/js/ScriptFunctionCall.cpp:40 > > +#ifndef NDEBUG > > Do we have debug-related behavior here? It's weird, it's not my code. It'll cleaned in the new patch. > > Source/WebCore/inspector/InjectedScriptManager.h:71 > > + explicit InjectedScriptManager(InspectedStateAccessCheck, bool = false); > > bool isForWorker = false ok. Created attachment 165341 [details]
how to reproduce this bug
Comment on attachment 161213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161213&action=review > Source/WebCore/inspector/InjectedScriptManager.h:87 > +protected: private? Created attachment 165344 [details]
Patch
JSMainThreadExecState is a wrapper of JSC API, it's seems designed just for main thread but not worker thread, so there are several assert statements in it. In worker inspector related code, there are several mistakes that invoke interfaces of JSMainThreadExecState from worker thread. (In reply to comment #6) > (From update of attachment 161213 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161213&action=review > > > Source/WebCore/inspector/InjectedScriptManager.h:87 > > +protected: > > private? yes (I really cannot remember why I declared it as "protected"), it should be "private", thx. (In reply to comment #7) > Created an attachment (id=165344) [details] > Patch The related code are already changed a lot since the first time I uploaded this patch. I'll make a new patch that can work with current code. Comment on attachment 165344 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165344&action=review > Source/WebCore/inspector/InjectedScriptManager.h:87 > + bool m_isForWorker; You don't seem to need this field. Please remove. Created attachment 165502 [details]
Patch
Comment on attachment 165502 [details]
Patch
commit
Comment on attachment 165502 [details] Patch Clearing flags on attachment: 165502 Committed r129476: <http://trac.webkit.org/changeset/129476> All reviewed patches have been landed. Closing bug. |