Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(624)

Issue 1856203002: [V8] Added DebugEvaluateContext to DCHECKs in contexts.cc (Closed)

Created:
4 years, 8 months ago by kozy
Modified:
4 years, 8 months ago
Reviewers:
Yang
CC:
v8-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[V8] Added DebugEvaluateContext to DCHECKs in contexts.cc DebugEvaluate context was introduced in [1]. With this CL DevTools has started to crash in console-save-to-temp-var.html test on debug bots. IsDebugEvaluateContext alternative is added to three DCHECKs in contexts.cc to avoid this crash. [1] https://chromium.googlesource.com/v8/v8/+/297daf6c37d6e4145e8aaec12ccc9762ac180850 BUG=chromium:599662 R=yangguo@chromium.org

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+5 lines, -4 lines) Patch
M src/contexts.cc View 1 chunk +5 lines, -4 lines 2 comments Download

Messages

Total messages: 4 (1 generated)
kozy
Yang, please take a look. Probably some additions changed are required.
4 years, 8 months ago (2016-04-04 23:57:10 UTC) #1
Yang
I don't think this is the correct fix. I came up with another: https://codereview.chromium.org/1859033002 Unfortunately ...
4 years, 8 months ago (2016-04-05 08:28:01 UTC) #2
kozy
4 years, 8 months ago (2016-04-06 04:53:40 UTC) #4
Message was sent while issue was closed.
On 2016/04/05 08:28:01, Yang wrote:
> I don't think this is the correct fix.
> 
> I came up with another: https://codereview.chromium.org/1859033002
> 
> Unfortunately I currently have no way to run layout tests. Could you verify
> whether this fixes the crash?
> 
> https://codereview.chromium.org/1856203002/diff/1/src/contexts.cc
> File src/contexts.cc (right):
> 
> https://codereview.chromium.org/1856203002/diff/1/src/contexts.cc#newcode94
> src/contexts.cc:94: IsDebugEvaluateContext());
> I don't think this can happen. We don't even expect a with-context at this
> point.
> 
> https://codereview.chromium.org/1856203002/diff/1/src/contexts.cc#newcode109
> src/contexts.cc:109: IsBlockContext() || IsDebugEvaluateContext());
> This should not happen. The only place I can see this being called with a
> debug-evaluate context is from the ScopeIterator. It should however not expose
> debug-evaluate contexts. Otherwise it might be confusing for the user.

I've checked test with your CL, it works. Thank you!

Powered by Google App Engine
This is Rietveld 408576698