Skip to content

Commit

Permalink
Revert of Use v8::MicrotasksScope internally in V8RecursionScope. (pa…
Browse files Browse the repository at this point in the history
…tchset chromium#6 id:100001 of https://codereview.chromium.org/1743763004/ )

Reason for revert:
This CL appears to be causing test failures on Linux ChromiumOS Tests (dbg)(1).

See:
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/12424

Tests are failing with:
Fatal error in ../../v8/src/api.cc, line 166
Check failed: handle_scope_implementer->GetMicrotasksScopeDepth() || !handle_scope_implementer->DebugMicrotasksScopeDepthIsZero()

Original issue's description:
> Use v8::MicrotasksScope internally in V8RecursionScope.
>
> If this sticks we can just remove V8RecursionScope and WebScopedMicrotaskSuppression,
> along with other cleanups.
>
> Attempt chromium#2. Previous one broke GinJavaBridgeValueConverterTest.TypedArrays.
>
> BUG=585949
>
> Committed: https://crrev.com/95a3bd544fe93629b209797d3251423f3d674463
> Cr-Commit-Position: refs/heads/master@{#380033}

TBR=jochen@chromium.org,adamk@chromium.org,dgozman@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=585949

Review URL: https://codereview.chromium.org/1777183002

Cr-Commit-Position: refs/heads/master@{#380356}
  • Loading branch information
tgsergeant authored and Commit bot committed Mar 10, 2016
1 parent 9f3b206 commit 3482857
Show file tree
Hide file tree
Showing 25 changed files with 169 additions and 108 deletions.
4 changes: 0 additions & 4 deletions chrome/test/base/v8_unit_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#include "base/strings/stringprintf.h"
#include "chrome/common/chrome_paths.h"
#include "third_party/WebKit/public/web/WebKit.h"
#include "third_party/WebKit/public/web/WebScopedMicrotaskSuppression.h"

namespace {

Expand Down Expand Up @@ -96,7 +95,6 @@ bool V8UnitTest::RunJavascriptTestF(const std::string& test_fixture,
v8::Local<v8::Context> context =
v8::Local<v8::Context>::New(isolate, context_);
v8::Context::Scope context_scope(context);
blink::WebScopedMicrotaskSuppression microtasks_scope;

v8::Local<v8::Value> function_property =
context->Global()->Get(v8::String::NewFromUtf8(isolate, "runTest"));
Expand Down Expand Up @@ -211,7 +209,6 @@ void V8UnitTest::ExecuteScriptInContext(const base::StringPiece& script_source,
v8::Local<v8::Context> context =
v8::Local<v8::Context>::New(isolate, context_);
v8::Context::Scope context_scope(context);
blink::WebScopedMicrotaskSuppression microtasks_scope;
v8::Local<v8::String> source =
v8::String::NewFromUtf8(isolate,
script_source.data(),
Expand Down Expand Up @@ -260,7 +257,6 @@ void V8UnitTest::TestFunction(const std::string& function_name) {
v8::Local<v8::Context> context =
v8::Local<v8::Context>::New(isolate, context_);
v8::Context::Scope context_scope(context);
blink::WebScopedMicrotaskSuppression microtasks_scope;

v8::Local<v8::Value> function_property = context->Global()->Get(
v8::String::NewFromUtf8(isolate, function_name.c_str()));
Expand Down
8 changes: 0 additions & 8 deletions content/child/v8_value_converter_impl_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
#include "base/values.h"
#include "content/child/v8_value_converter_impl.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/WebKit/public/web/WebScopedMicrotaskSuppression.h"
#include "v8/include/v8.h"

namespace content {
Expand Down Expand Up @@ -288,7 +287,6 @@ TEST_F(V8ValueConverterImplTest, ObjectExceptions) {
v8::Local<v8::Context> context =
v8::Local<v8::Context>::New(isolate_, context_);
v8::Context::Scope context_scope(context);
blink::WebScopedMicrotaskSuppression microtasks_scope;

// Set up objects to throw when reading or writing 'foo'.
const char* source =
Expand Down Expand Up @@ -331,7 +329,6 @@ TEST_F(V8ValueConverterImplTest, ArrayExceptions) {
v8::Local<v8::Context> context =
v8::Local<v8::Context>::New(isolate_, context_);
v8::Context::Scope context_scope(context);
blink::WebScopedMicrotaskSuppression microtasks_scope;

const char* source = "(function() {"
"var arr = [];"
Expand Down Expand Up @@ -408,7 +405,6 @@ TEST_F(V8ValueConverterImplTest, Prototype) {
v8::Local<v8::Context> context =
v8::Local<v8::Context>::New(isolate_, context_);
v8::Context::Scope context_scope(context);
blink::WebScopedMicrotaskSuppression microtasks_scope;

const char* source = "(function() {"
"Object.prototype.foo = 'foo';"
Expand All @@ -433,7 +429,6 @@ TEST_F(V8ValueConverterImplTest, StripNullFromObjects) {
v8::Local<v8::Context> context =
v8::Local<v8::Context>::New(isolate_, context_);
v8::Context::Scope context_scope(context);
blink::WebScopedMicrotaskSuppression microtasks_scope;

const char* source = "(function() {"
"return { foo: undefined, bar: null };"
Expand Down Expand Up @@ -492,7 +487,6 @@ TEST_F(V8ValueConverterImplTest, WeirdProperties) {
v8::Local<v8::Context> context =
v8::Local<v8::Context>::New(isolate_, context_);
v8::Context::Scope context_scope(context);
blink::WebScopedMicrotaskSuppression microtasks_scope;

const char* source = "(function() {"
"return {"
Expand Down Expand Up @@ -531,7 +525,6 @@ TEST_F(V8ValueConverterImplTest, ArrayGetters) {
v8::Local<v8::Context> context =
v8::Local<v8::Context>::New(isolate_, context_);
v8::Context::Scope context_scope(context);
blink::WebScopedMicrotaskSuppression microtasks_scope;

const char* source = "(function() {"
"var a = [0];"
Expand All @@ -556,7 +549,6 @@ TEST_F(V8ValueConverterImplTest, UndefinedValueBehavior) {
v8::Local<v8::Context> context =
v8::Local<v8::Context>::New(isolate_, context_);
v8::Context::Scope context_scope(context);
blink::WebScopedMicrotaskSuppression microtasks_scope;

v8::Local<v8::Object> object;
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
#include "content/common/android/gin_java_bridge_value.h"
#include "content/renderer/java/gin_java_bridge_value_converter.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/WebKit/public/web/WebScopedMicrotaskSuppression.h"
#include "v8/include/v8.h"

namespace content {
Expand Down Expand Up @@ -100,7 +99,6 @@ TEST_F(GinJavaBridgeValueConverterTest, TypedArrays) {
v8::Local<v8::Context> context =
v8::Local<v8::Context>::New(isolate_, context_);
v8::Context::Scope context_scope(context);
blink::WebScopedMicrotaskSuppression microtasks_scope;

scoped_ptr<GinJavaBridgeValueConverter> converter(
new GinJavaBridgeValueConverter());
Expand Down
2 changes: 0 additions & 2 deletions content/renderer/pepper/v8_var_converter_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
#include "ppapi/shared_impl/var.h"
#include "ppapi/shared_impl/var_tracker.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/WebKit/public/web/WebScopedMicrotaskSuppression.h"
#include "v8/include/v8.h"

using ppapi::ArrayBufferVar;
Expand Down Expand Up @@ -404,7 +403,6 @@ TEST_F(V8VarConverterTest, StrangeDictionaryKeyTest) {
v8::Local<v8::Context> context =
v8::Local<v8::Context>::New(isolate_, context_);
v8::Context::Scope context_scope(context);
blink::WebScopedMicrotaskSuppression microtasks_scope;

const char* source =
"(function() {"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
#include "base/values.h"
#include "extensions/renderer/activity_log_converter_strategy.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/WebKit/public/web/WebScopedMicrotaskSuppression.h"
#include "v8/include/v8.h"

using content::V8ValueConverter;
Expand Down Expand Up @@ -123,7 +122,6 @@ TEST_F(ActivityLogConverterStrategyTest, ConversionTest) {
"};"
"})();";

blink::WebScopedMicrotaskSuppression microtasks_scope;
v8::Local<v8::Script> script(
v8::Script::Compile(v8::String::NewFromUtf8(isolate_, source)));
v8::Local<v8::Object> v8_object = script->Run().As<v8::Object>();
Expand Down
2 changes: 1 addition & 1 deletion extensions/renderer/api_test_base.cc
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ void ApiTestEnvironment::RunTestInner(const std::string& test_name,
}

void ApiTestEnvironment::RunPromisesAgain() {
v8::MicrotasksScope::PerformCheckpoint(env()->isolate());
env()->isolate()->RunMicrotasks();
base::MessageLoop::current()->PostTask(
FROM_HERE, base::Bind(&ApiTestEnvironment::RunPromisesAgain,
base::Unretained(this)));
Expand Down
2 changes: 1 addition & 1 deletion extensions/renderer/module_system_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ void ModuleSystemTest::ExpectNoAssertionsMade() {
}

void ModuleSystemTest::RunResolvedPromises() {
v8::MicrotasksScope::PerformCheckpoint(isolate_);
isolate_->RunMicrotasks();
}

} // namespace extensions
2 changes: 0 additions & 2 deletions extensions/renderer/safe_builtins.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
#include "base/strings/stringprintf.h"
#include "extensions/renderer/script_context.h"
#include "extensions/renderer/v8_helpers.h"
#include "third_party/WebKit/public/web/WebScopedMicrotaskSuppression.h"

namespace extensions {

Expand Down Expand Up @@ -201,7 +200,6 @@ class ExtensionImpl : public v8::Extension {
return;
}

blink::WebScopedMicrotaskSuppression microtasks_scope;
v8::Local<v8::Value> return_value;
if (function->Call(context, recv, argc, argv.get()).ToLocal(&return_value))
info.GetReturnValue().Set(return_value);
Expand Down
2 changes: 1 addition & 1 deletion gin/run_microtasks_observer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ void RunMicrotasksObserver::WillProcessTask(const base::PendingTask& task) {

void RunMicrotasksObserver::DidProcessTask(const base::PendingTask& task) {
v8::Isolate::Scope scope(isolate_);
v8::MicrotasksScope::PerformCheckpoint(isolate_);
isolate_->RunMicrotasks();
}

} // namespace gin
2 changes: 1 addition & 1 deletion gin/run_microtasks_observer.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ namespace gin {

// Runs any pending v8 Microtasks each time a task is completed.
// TODO(hansmuller); At some point perhaps this can be replaced with
// the (currently experimental) v8::MicrotasksPolicy::kAuto method.
// the (currently experimental) Isolate::SetAutorunMicrotasks() method.

class RunMicrotasksObserver : public base::MessageLoop::TaskObserver {
public:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
#include "bindings/core/v8/V8BindingForTesting.h"
#include "bindings/core/v8/V8BindingMacros.h"
#include "bindings/core/v8/V8IteratorResultValue.h"
#include "bindings/core/v8/V8RecursionScope.h"
#include "bindings/core/v8/V8ThrowException.h"
#include "core/dom/Document.h"
#include "core/streams/ReadableStreamController.h"
Expand Down Expand Up @@ -139,7 +138,7 @@ class ReadableStreamOperationsTest : public ::testing::Test {
~ReadableStreamOperationsTest() override
{
// Execute all pending microtasks
v8::MicrotasksScope::PerformCheckpoint(isolate());
isolate()->RunMicrotasks();
EXPECT_FALSE(m_block.HasCaught());
}

Expand All @@ -150,7 +149,6 @@ class ReadableStreamOperationsTest : public ::testing::Test {
{
v8::Local<v8::String> source;
v8::Local<v8::Script> script;
V8RecursionScope::MicrotaskSuppression microtasks(isolate());
if (!v8Call(v8::String::NewFromUtf8(isolate(), s, v8::NewStringType::kNormal), source)) {
ADD_FAILURE();
return ScriptValue();
Expand Down Expand Up @@ -255,20 +253,20 @@ TEST_F(ReadableStreamOperationsTest, Read)
Function::createFunction(getScriptState(), it2),
NotReached::createFunction(getScriptState()));

v8::MicrotasksScope::PerformCheckpoint(isolate());
isolate()->RunMicrotasks();
EXPECT_FALSE(it1->isSet());
EXPECT_FALSE(it2->isSet());

ASSERT_FALSE(evalWithPrintingError("controller.enqueue('hello')").isEmpty());
v8::MicrotasksScope::PerformCheckpoint(isolate());
isolate()->RunMicrotasks();
EXPECT_TRUE(it1->isSet());
EXPECT_TRUE(it1->isValid());
EXPECT_FALSE(it1->isDone());
EXPECT_EQ("hello", it1->value());
EXPECT_FALSE(it2->isSet());

ASSERT_FALSE(evalWithPrintingError("controller.close()").isEmpty());
v8::MicrotasksScope::PerformCheckpoint(isolate());
isolate()->RunMicrotasks();
EXPECT_TRUE(it1->isSet());
EXPECT_TRUE(it1->isValid());
EXPECT_FALSE(it1->isDone());
Expand Down Expand Up @@ -311,7 +309,7 @@ TEST_F(ReadableStreamOperationsTest, CreateReadableStreamWithCustomUnderlyingSou
ReadableStreamOperations::read(getScriptState(), reader).then(Function::createFunction(getScriptState(), it2), NotReached::createFunction(getScriptState()));
ReadableStreamOperations::read(getScriptState(), reader).then(Function::createFunction(getScriptState(), it3), NotReached::createFunction(getScriptState()));

v8::MicrotasksScope::PerformCheckpoint(isolate());
isolate()->RunMicrotasks();

EXPECT_EQ(10, underlyingSource->desiredSize());

Expand All @@ -328,7 +326,7 @@ TEST_F(ReadableStreamOperationsTest, CreateReadableStreamWithCustomUnderlyingSou
EXPECT_FALSE(it3->isSet());

underlyingSource->close();
v8::MicrotasksScope::PerformCheckpoint(isolate());
isolate()->RunMicrotasks();

EXPECT_TRUE(it3->isSet());
EXPECT_TRUE(it3->isValid());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ TEST_F(ScriptPromisePropertyGarbageCollectedTest, Resolve_ResolvesScriptPromise)
getProperty()->resolve(value);
EXPECT_EQ(Property::Resolved, getProperty()->getState());

v8::MicrotasksScope::PerformCheckpoint(isolate());
isolate()->RunMicrotasks();
EXPECT_EQ(1u, nResolveCalls);
EXPECT_EQ(1u, nOtherResolveCalls);
EXPECT_EQ(wrap(mainWorld(), value), actual);
Expand All @@ -307,7 +307,7 @@ TEST_F(ScriptPromisePropertyGarbageCollectedTest, ResolveAndGetPromiseOnOtherWor
getProperty()->resolve(value);
EXPECT_EQ(Property::Resolved, getProperty()->getState());

v8::MicrotasksScope::PerformCheckpoint(isolate());
isolate()->RunMicrotasks();
EXPECT_EQ(1u, nResolveCalls);
EXPECT_EQ(0u, nOtherResolveCalls);

Expand All @@ -316,7 +316,7 @@ TEST_F(ScriptPromisePropertyGarbageCollectedTest, ResolveAndGetPromiseOnOtherWor
otherPromise.then(stub(currentScriptState(), otherActual, nOtherResolveCalls), notReached(currentScriptState()));
}

v8::MicrotasksScope::PerformCheckpoint(isolate());
isolate()->RunMicrotasks();
EXPECT_EQ(1u, nResolveCalls);
EXPECT_EQ(1u, nOtherResolveCalls);
EXPECT_EQ(wrap(mainWorld(), value), actual);
Expand All @@ -343,7 +343,7 @@ TEST_F(ScriptPromisePropertyGarbageCollectedTest, Reject_RejectsScriptPromise)
getProperty()->promise(otherWorld()).then(notReached(currentScriptState()), stub(currentScriptState(), otherActual, nOtherRejectCalls));
}

v8::MicrotasksScope::PerformCheckpoint(isolate());
isolate()->RunMicrotasks();
EXPECT_EQ(1u, nRejectCalls);
EXPECT_EQ(wrap(mainWorld(), reason), actual);
EXPECT_EQ(1u, nOtherRejectCalls);
Expand Down Expand Up @@ -374,7 +374,7 @@ TEST_F(ScriptPromisePropertyGarbageCollectedTest, Resolve_DeadContext)
getProperty()->resolve(new GarbageCollectedScriptWrappable("value"));
EXPECT_EQ(Property::Pending, getProperty()->getState());

v8::MicrotasksScope::PerformCheckpoint(v8::Isolate::GetCurrent());
v8::Isolate::GetCurrent()->RunMicrotasks();
}

TEST_F(ScriptPromisePropertyGarbageCollectedTest, Reset)
Expand Down Expand Up @@ -405,7 +405,7 @@ TEST_F(ScriptPromisePropertyGarbageCollectedTest, Reset)
EXPECT_EQ(0u, nOldResolveCalls);
EXPECT_EQ(0u, nNewRejectCalls);

v8::MicrotasksScope::PerformCheckpoint(isolate());
isolate()->RunMicrotasks();
EXPECT_EQ(1u, nOldResolveCalls);
EXPECT_EQ(1u, nNewRejectCalls);
EXPECT_NE(oldPromise, newPromise);
Expand Down Expand Up @@ -445,7 +445,7 @@ TEST_F(ScriptPromisePropertyRefCountedTest, Resolve)
getProperty()->resolve(value.get());
EXPECT_EQ(Property::Resolved, getProperty()->getState());

v8::MicrotasksScope::PerformCheckpoint(isolate());
isolate()->RunMicrotasks();
EXPECT_EQ(1u, nResolveCalls);
EXPECT_EQ(wrap(mainWorld(), value), actual);
}
Expand All @@ -464,7 +464,7 @@ TEST_F(ScriptPromisePropertyRefCountedTest, Reject)
getProperty()->reject(reason);
EXPECT_EQ(Property::Rejected, getProperty()->getState());

v8::MicrotasksScope::PerformCheckpoint(isolate());
isolate()->RunMicrotasks();
EXPECT_EQ(1u, nRejectCalls);
EXPECT_EQ(wrap(mainWorld(), reason), actual);
}
Expand Down Expand Up @@ -514,7 +514,7 @@ class ScriptPromisePropertyNonScriptWrappableResolutionTargetTest : public Scrip
property->promise(DOMWrapperWorld::mainWorld()).then(stub(currentScriptState(), actualValue, nResolveCalls), notReached(currentScriptState()));
}
property->resolve(value);
v8::MicrotasksScope::PerformCheckpoint(isolate());
isolate()->RunMicrotasks();
{
ScriptState::Scope scope(mainScriptState());
actual = toCoreString(actualValue.v8Value()->ToString(mainScriptState()->context()).ToLocalChecked());
Expand Down
Loading

0 comments on commit 3482857

Please sign in to comment.