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

Issue 1131413002: Revert of Add support for new-ish V8 use counters (Closed)

Created:
5 years, 7 months ago by sof
Modified:
5 years, 7 months ago
CC:
arv+blink, blink-reviews, blink-reviews-bindings_chromium.org, vivekg_samsung, vivekg
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Revert of Add support for new-ish V8 use counters (patchset #2 id:20001 of https://codereview.chromium.org/1135753002/) Reason for revert: The v8 use counters now being handled are reported over an Isolate for which callingExecutionContext() fails -- https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac_Oilpan__dbg_/9995/layout-test-results/results.html (first build on Mac Oilpan(dbg) with the new crashes.) Original issue's description: > Add support for new-ish V8 use counters > > R=jochen@chromium.org > BUG= > > Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195119 TBR=jochen@chromium.org,erikcorry@chromium.org,erik.corry@gmail.com NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195151

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+0 lines, -16 lines) Patch
M Source/bindings/core/v8/V8PerIsolateData.cpp View 1 chunk +0 lines, -12 lines 0 comments Download
M Source/core/frame/UseCounter.h View 1 chunk +0 lines, -4 lines 0 comments Download

Messages

Total messages: 5 (0 generated)
sof
Created Revert of Add support for new-ish V8 use counters
5 years, 7 months ago (2015-05-09 14:01:10 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1131413002/1
5 years, 7 months ago (2015-05-09 14:01:36 UTC) #2
commit-bot: I haz the power
Committed patchset #1 (id:1) as https://src.chromium.org/viewvc/blink?view=rev&revision=195151
5 years, 7 months ago (2015-05-09 14:02:06 UTC) #3
haraken
LGTM (BTW, I don't fully understand why these UseCounters are using callingExecutionContext, not currentExecutionContext. I ...
5 years, 7 months ago (2015-05-09 14:33:03 UTC) #4
Erik Corry
5 years, 7 months ago (2015-05-10 23:08:00 UTC) #5
Message was sent while issue was closed.
Thanks for reverting, Sigbjörn!

On 2015/05/09 14:33:03, haraken wrote:
> LGTM
> 
> (BTW, I don't fully understand why these UseCounters are using
> callingExecutionContext, not currentExecutionContext. I discussed this with
> adamk@ before but couldn't figure out why.)

I don't understand this either, but I think that's not the issue here.  Looking
at the stack trace below there are two issues:

1) It's a bad idea to do a callback in the middle of GC.  The embedder may
choose to do all sorts of stuff, and some of that doesn't work in the middle of
GC.  This was my mistake.
2) We don't put a HandleScope on the stack when we do the use-counter callback,
but the embedder expects that we did.

Perhaps we normally got away with 2) until now because there's usually a
HandleScope somewhere for the times when we used to increment the counters.

I created https://codereview.chromium.org/1125383007

crash log for devtools (pid <unknown>):
STDOUT: #CRASHED - devtools
STDERR: 1   0x10d33119a blink::reportFatalErrorInMainThread(char const*, char
const*)
STDERR: 2   0x1210c574f v8::Utils::ReportApiFailure(char const*, char const*)
STDERR: 3   0x12134de48
v8::internal::HandleScope::Extend(v8::internal::Isolate*)
STDERR: 4   0x1214c93a7 v8::internal::Isolate::GetCallingNativeContext()
STDERR: 5   0x1210ff06e v8::Isolate::GetCallingContext()
STDERR: 6   0x10d30f913 blink::callingExecutionContext(v8::Isolate*)
STDERR: 7   0x10d353dcb blink::useCounterCallback(v8::Isolate*,
v8::Isolate::UseCounterFeature)
STDERR: 8   0x1213c432d
v8::internal::MarkCompactCollector::EvictPopularEvacuationCandidate(v8::internal::Page*)
STDERR: 9   0x1213b3c86
v8::internal::MarkCompactCollector::ProcessAndClearWeakCells()
STDERR: 10  0x1213b2966 v8::internal::MarkCompactCollector::CollectGarbage()
STDERR: 11  0x1213746d4 v8::internal::Heap::MarkCompact()
STDERR: 12  0x1213730a4
v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector,
v8::GCCallbackFlags)
STDERR: 13  0x121372955
v8::internal::Heap::CollectGarbage(v8::internal::GarbageCollector, char const*,
char const*, v8::GCCallbackFlags)
STDERR: 14  0x121371c36 v8::internal::Heap::HandleGCRequest()
STDERR: 15  0x121306cd6 v8::internal::StackGuard::HandleInterrupts()
STDERR: 16  0x121657c9e v8::internal::Runtime_StackGuard(int,
v8::internal::Object**, v8::internal::Isolate*)
STDERR: 17  0x3cf05e407f9b

Powered by Google App Engine
This is Rietveld 408576698