Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2761293002:
Correctly annotate v8::StackTrace and v8::StackFrame API methods (Closed)
DescriptionCorrectly annotate v8::StackTrace and v8::StackFrame API methods
BUG=v8:5830
Review-Url: https://codereview.chromium.org/2761293002
Cr-Commit-Position: refs/heads/master@{#44101}
Committed: https://chromium.googlesource.com/v8/v8/+/c10cde195983dce0b88eeb533774a4fe14a8813f
Patch Set 1 #
Total comments: 10
Patch Set 2 : Move kExposeFramesAcrossSecurityOrigins flag to v8-stack-trace-impl.cc #Patch Set 3 : Rebase #
Messages
Total messages: 22 (14 generated)
Description was changed from ========== Correctly annotate v8::StackTrace and v8::StackFrame API methods BUG=5830 ========== to ========== Correctly annotate v8::StackTrace and v8::StackFrame API methods BUG=5830 ==========
mmoroz@chromium.org changed reviewers: + jochen@chromium.org, yangguo@chromium.org
https://codereview.chromium.org/2761293002/diff/1/src/api.cc File src/api.cc (left): https://codereview.chromium.org/2761293002/diff/1/src/api.cc#oldcode2869 src/api.cc:2869: // TODO(dcarney): remove when ScriptDebugServer is fixed. Looks like dcarney@ is not at Google anymore. Should we go through other his TODO / FIXME comments? There are ~35 of them. https://codereview.chromium.org/2761293002/diff/1/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2761293002/diff/1/src/api.cc#newcode2870 src/api.cc:2870: static_cast<int>(options) | kExposeFramesAcrossSecurityOrigins); Why do we need to expose frames across security origins? I've tried to remove this, and lots of tests failed after that. So it feels that this is important flag, but I don't understand the logic behind it. Why do we want to expose cross-origin stack frames? https://codereview.chromium.org/2761293002/diff/1/src/api.cc#newcode2886 src/api.cc:2886: i::JSReceiver::GetProperty(isolate, self, propertyName).ToHandleChecked(); None of the tests failed, but I cannot understand whether GetProperty() can execute script or not? https://codereview.chromium.org/2761293002/diff/1/src/api.cc#newcode2942 src/api.cc:2942: return obj->IsTrue(isolate); This concerns me a bit. Can "IsTrue" evaluate the value, i.e. execute some script?
can you trigger tryjobs? https://codereview.chromium.org/2761293002/diff/1/src/api.cc File src/api.cc (left): https://codereview.chromium.org/2761293002/diff/1/src/api.cc#oldcode2869 src/api.cc:2869: // TODO(dcarney): remove when ScriptDebugServer is fixed. On 2017/03/21 at 13:12:58, mmoroz wrote: > Looks like dcarney@ is not at Google anymore. Should we go through other his TODO / FIXME comments? There are ~35 of them. yeah, eventually we should do that... however, we'll need to address them / assign to a new owner. Just deleting is no good :/ https://codereview.chromium.org/2761293002/diff/1/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2761293002/diff/1/src/api.cc#newcode2870 src/api.cc:2870: static_cast<int>(options) | kExposeFramesAcrossSecurityOrigins); On 2017/03/21 at 13:12:58, mmoroz wrote: > Why do we need to expose frames across security origins? I've tried to remove this, and lots of tests failed after that. So it feels that this is important flag, but I don't understand the logic behind it. Why do we want to expose cross-origin stack frames? when we expose a stack to javascript, e.g. via new Error().stack, we can't expose cross origin frames. The debugger, however, will want to expose such stacks via devtools.
The CQ bit was checked by mmoroz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/37319)
Description was changed from ========== Correctly annotate v8::StackTrace and v8::StackFrame API methods BUG=5830 ========== to ========== Correctly annotate v8::StackTrace and v8::StackFrame API methods BUG=v8:5830 ==========
Thanks for the answers, Jochen! https://codereview.chromium.org/2761293002/diff/1/src/api.cc File src/api.cc (left): https://codereview.chromium.org/2761293002/diff/1/src/api.cc#oldcode2869 src/api.cc:2869: // TODO(dcarney): remove when ScriptDebugServer is fixed. On 2017/03/22 09:59:06, jochen wrote: > On 2017/03/21 at 13:12:58, mmoroz wrote: > > Looks like dcarney@ is not at Google anymore. Should we go through other his > TODO / FIXME comments? There are ~35 of them. > > yeah, eventually we should do that... however, we'll need to address them / > assign to a new owner. Just deleting is no good :/ Yeah, I didn't mean to simply delete others. Sorry, I should have been more clear. I can try to go through TODOs and address those ones that I can resolve + reassign all the rest. https://codereview.chromium.org/2761293002/diff/1/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2761293002/diff/1/src/api.cc#newcode2870 src/api.cc:2870: static_cast<int>(options) | kExposeFramesAcrossSecurityOrigins); On 2017/03/22 09:59:06, jochen wrote: > On 2017/03/21 at 13:12:58, mmoroz wrote: > > Why do we need to expose frames across security origins? I've tried to remove > this, and lots of tests failed after that. So it feels that this is important > flag, but I don't understand the logic behind it. Why do we want to expose > cross-origin stack frames? > > when we expose a stack to javascript, e.g. via new Error().stack, we can't > expose cross origin frames. The debugger, however, will want to expose such > stacks via devtools. Acknowledged.
https://codereview.chromium.org/2761293002/diff/1/src/api.cc File src/api.cc (left): https://codereview.chromium.org/2761293002/diff/1/src/api.cc#oldcode2869 src/api.cc:2869: // TODO(dcarney): remove when ScriptDebugServer is fixed. On 2017/03/22 at 11:55:43, mmoroz wrote: > On 2017/03/22 09:59:06, jochen wrote: > > On 2017/03/21 at 13:12:58, mmoroz wrote: > > > Looks like dcarney@ is not at Google anymore. Should we go through other his > > TODO / FIXME comments? There are ~35 of them. > > > > yeah, eventually we should do that... however, we'll need to address them / > > assign to a new owner. Just deleting is no good :/ > > Yeah, I didn't mean to simply delete others. Sorry, I should have been more clear. I can try to go through TODOs and address those ones that I can resolve + reassign all the rest. great! I'd do that in separate CLs, however. in this instance, you can probably move kExposeFramesAcrossSecurityOrigins to https://cs.chromium.org/chromium/src/v8/src/inspector/v8-stack-trace-impl.cc?...
https://codereview.chromium.org/2761293002/diff/1/src/api.cc File src/api.cc (left): https://codereview.chromium.org/2761293002/diff/1/src/api.cc#oldcode2869 src/api.cc:2869: // TODO(dcarney): remove when ScriptDebugServer is fixed. On 2017/03/22 12:45:33, jochen wrote: > On 2017/03/22 at 11:55:43, mmoroz wrote: > > On 2017/03/22 09:59:06, jochen wrote: > > > On 2017/03/21 at 13:12:58, mmoroz wrote: > > > > Looks like dcarney@ is not at Google anymore. Should we go through other > his > > > TODO / FIXME comments? There are ~35 of them. > > > > > > yeah, eventually we should do that... however, we'll need to address them / > > > assign to a new owner. Just deleting is no good :/ > > > > Yeah, I didn't mean to simply delete others. Sorry, I should have been more > clear. I can try to go through TODOs and address those ones that I can resolve + > reassign all the rest. > > great! I'd do that in separate CLs, however. > > in this instance, you can probably move kExposeFramesAcrossSecurityOrigins to > https://cs.chromium.org/chromium/src/v8/src/inspector/v8-stack-trace-impl.cc?... Done.
The CQ bit was checked by mmoroz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by mmoroz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490357530119720,
"parent_rev": "4693a4337a5fdff6645786518e560cd476c765d8", "commit_rev":
"c10cde195983dce0b88eeb533774a4fe14a8813f"}
Message was sent while issue was closed.
Description was changed from ========== Correctly annotate v8::StackTrace and v8::StackFrame API methods BUG=v8:5830 ========== to ========== Correctly annotate v8::StackTrace and v8::StackFrame API methods BUG=v8:5830 Review-Url: https://codereview.chromium.org/2761293002 Cr-Commit-Position: refs/heads/master@{#44101} Committed: https://chromium.googlesource.com/v8/v8/+/c10cde195983dce0b88eeb533774a4fe14a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/c10cde195983dce0b88eeb533774a4fe14a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
