Skip to content

Commit

Permalink
Revert 170357 "Revert of Make DOMWrapperWorld::current() return ..."
Browse files Browse the repository at this point in the history
This is a revert of the revert of r170261.
r170261 was reverted because it introduced new memory leaks, but the leaks were false-positive.

> Revert of Make DOMWrapperWorld::current() return a reference instead of a pointer (https://codereview.chromium.org/209713003/)
> 
> Also reverting dependent patch https://codereview.chromium.org/213543004
> 
> Reason for revert:
> Likely to have caused memory leaks:
> http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%2BLSan%20Tests%20%282%29/builds/989/steps/browser_tests/logs/stdio#failure1
> 
> (1)
> Direct leak of 32 byte(s) in 1 object(s) allocated from:
>     #0 0x4b50d1 in operator new(unsigned long) /usr/local/google/home/hwennborg/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:62
>     #1 0x9bdfd8a in WebCore::V8PerContextDataHolder::install(v8::Handle<v8::Context>, WTF::PassRefPtr<WebCore::DOMWrapperWorld>) third_party/WebKit/Source/bindings/v8/V8PerContextData.cpp:56
>     #2 0x9bdfa06 in WebCore::V8PerContextData::V8PerContextData(v8::Handle<v8::Context>, WTF::PassRefPtr<WebCore::DOMWrapperWorld>) third_party/WebKit/Source/bindings/v8/V8PerContextData.cpp:108
>     #3 0x9bc1afd in WebCore::V8PerContextData::create(v8::Handle<v8::Context>, WTF::PassRefPtr<WebCore::DOMWrapperWorld>) third_party/WebKit/Source/bindings/v8/V8PerContextData.h:66:16
>     #4 0x9be64fc in WebCore::V8PerIsolateData::ensureDomInJSContext() third_party/WebKit/Source/bindings/v8/V8PerIsolateData.cpp:132
> 
> (2)
> Indirect leak of 40 byte(s) in 1 object(s) allocated from:
>     #0 0x4b50d1 in operator new(unsigned long) /usr/local/google/home/hwennborg/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:62
>     #1 0x9b2ee18 in operator new third_party/WebKit/Source/bindings/v8/DOMWrapperWorld.cpp:59:22
>     #2 0x9b2ee18 in WebCore::DOMWrapperWorld::create(int, int) third_party/WebKit/Source/bindings/v8/DOMWrapperWorld.cpp:53
>     #3 0x9be64e7 in WebCore::V8PerIsolateData::ensureDomInJSContext() third_party/WebKit/Source/bindings/v8/V8PerIsolateData.cpp:132
>  
> (3)
> Indirect leak of 24 byte(s) in 1 object(s) allocated from:
>     #0 0x4b46c1 in __interceptor_malloc /usr/local/google/home/hwennborg/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:75
>     #1 0x62caf0a in partitionAllocGenericFlags third_party/WebKit/Source/wtf/PartitionAlloc.h:533
>     #2 0x62caf0a in partitionAllocGeneric third_party/WebKit/Source/wtf/PartitionAlloc.h:549
>     #3 0x62caf0a in WTF::fastMalloc(unsigned long) third_party/WebKit/Source/wtf/FastMalloc.cpp:125
>     #4 0x9b2ed61 in operator new third_party/WebKit/Source/wtf/RefCounted.h:175
>     #5 0x9b2ed61 in WebCore::DOMWrapperWorld::create(int, int) third_party/WebKit/Source/bindings/v8/DOMWrapperWorld.cpp:53
> 
> Original issue's description:
> > Make DOMWrapperWorld::current() return a reference instead of a pointer
> > 
> > Now that it's guaranteed that DOMWrapperWorld is not 0 at any given time, we can make it a reference instead of a pointer.
> > 
> > BUG=341032
> > 
> > Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170261
> 
> TBR=jochen@chromium.org,dcarney@chromium.org,abarth@chromium.org,haraken@chromium.org
> NOTREECHECKS=true
> NOTRY=true
> BUG=341032
> 
> Review URL: https://codereview.chromium.org/217053007

TBR=enne@chromium.org

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

git-svn-id: svn://svn.chromium.org/blink/trunk@170424 bbb929c8-8fbe-4397-9dbb-9b2b20218538
  • Loading branch information
haraken@chromium.org committed Mar 31, 2014
1 parent 5e620ca commit 8961684
Show file tree
Hide file tree
Showing 118 changed files with 7,057 additions and 16,427 deletions.
2 changes: 1 addition & 1 deletion Source/bindings/templates/callback_interface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ namespace WebCore {

v8::HandleScope handleScope(m_isolate);

v8::Handle<v8::Context> v8Context = toV8Context(executionContext(), m_world.get());
v8::Handle<v8::Context> v8Context = toV8Context(executionContext(), *m_world);
if (v8Context.IsEmpty())
{{return_default}};

Expand Down
12 changes: 6 additions & 6 deletions Source/bindings/templates/interface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -662,7 +662,7 @@ static void constructor(const v8::FunctionCallbackInfo<v8::Value>& info)
thus passing it around would cause leakage.
2) Errors cannot be cloned (or serialized):
http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#safe-passing-of-structured-data #}
if (DOMWrapperWorld::current(isolate)->isIsolatedWorld()) {
if (DOMWrapperWorld::current(isolate).isIsolatedWorld()) {
{% for attribute in any_type_attributes %}
if (!{{attribute.name}}.IsEmpty())
event->setSerialized{{attribute.name | blink_capitalize}}(SerializedScriptValue::createAndSwallowExceptions({{attribute.name}}, isolate));
Expand Down Expand Up @@ -975,7 +975,7 @@ static void configure{{v8_class}}Template(v8::Handle<v8::FunctionTemplate> funct
{% filter conditional(method.conditional_string) %}
{% if method.is_do_not_check_security %}
{% if method.is_per_world_bindings %}
if (DOMWrapperWorld::current(isolate)->isMainWorld()) {
if (DOMWrapperWorld::current(isolate).isMainWorld()) {
{{install_do_not_check_security_signature(method, 'ForMainWorld')}}
} else {
{{install_do_not_check_security_signature(method)}}
Expand All @@ -985,7 +985,7 @@ static void configure{{v8_class}}Template(v8::Handle<v8::FunctionTemplate> funct
{% endif %}
{% else %}{# is_do_not_check_security #}
{% if method.is_per_world_bindings %}
if (DOMWrapperWorld::current(isolate)->isMainWorld()) {
if (DOMWrapperWorld::current(isolate).isMainWorld()) {
{% filter runtime_enabled(method.runtime_enabled_function) %}
{{install_custom_signature(method, 'ForMainWorld')}}
{% endfilter %}
Expand Down Expand Up @@ -1203,7 +1203,7 @@ EventTarget* {{v8_class}}::toEventTarget(v8::Handle<v8::Object> object)
{% if interface_name == 'Window' %}
v8::Handle<v8::ObjectTemplate> V8Window::getShadowObjectTemplate(v8::Isolate* isolate)
{
if (DOMWrapperWorld::current(isolate)->isMainWorld()) {
if (DOMWrapperWorld::current(isolate).isMainWorld()) {
DEFINE_STATIC_LOCAL(v8::Persistent<v8::ObjectTemplate>, V8WindowShadowObjectCacheForMainWorld, ());
if (V8WindowShadowObjectCacheForMainWorld.IsEmpty()) {
TRACE_EVENT_SCOPED_SAMPLING_STATE("Blink", "BuildDOMTemplate");
Expand Down Expand Up @@ -1244,8 +1244,8 @@ v8::Handle<v8::Object> wrap({{cpp_class}}* impl, v8::Handle<v8::Object> creation
{% if is_document %}
if (wrapper.IsEmpty())
return wrapper;
DOMWrapperWorld* world = DOMWrapperWorld::current(isolate);
if (world->isMainWorld()) {
DOMWrapperWorld& world = DOMWrapperWorld::current(isolate);
if (world.isMainWorld()) {
if (LocalFrame* frame = impl->frame())
frame->script().windowShell(world)->updateDocumentWrapper(wrapper);
}
Expand Down
2 changes: 1 addition & 1 deletion Source/bindings/templates/interface.h
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ inline void v8SetReturnValue(const CallbackInfo& callbackInfo, {{cpp_class}}* im
template<typename CallbackInfo>
inline void v8SetReturnValueForMainWorld(const CallbackInfo& callbackInfo, {{cpp_class}}* impl)
{
ASSERT(DOMWrapperWorld::current(callbackInfo.GetIsolate())->isMainWorld());
ASSERT(DOMWrapperWorld::current(callbackInfo.GetIsolate()).isMainWorld());
if (UNLIKELY(!impl)) {
v8SetReturnValueNull(callbackInfo);
return;
Expand Down
2 changes: 1 addition & 1 deletion Source/bindings/tests/idls/SVGTestInterface.idl
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,6 @@
[
DependentLifetime,
] interface SVGTestInterface {
[Reflect] attribute DOMString type; // Test SVGNames namespace
[Reflect] attribute DOMString type; // Test SVGNames namespace
// [Immutable] attribute SVGPoint immutablePoint; // [Immutable] is a nop
};
37 changes: 0 additions & 37 deletions Source/bindings/tests/idls/TestEventTarget.idl

This file was deleted.

2 changes: 1 addition & 1 deletion Source/bindings/tests/idls/TestImplements.idl
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
*/

[
NoInterfaceObject, // Always used on target of 'implements'
NoInterfaceObject, // Always used on target of 'implements'
] interface TestImplements {
static readonly attribute long implementsStaticReadOnlyLongAttribute;
static attribute DOMString implementsStaticStringAttribute;
Expand Down
4 changes: 2 additions & 2 deletions Source/bindings/tests/idls/TestImplements2.idl
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@
*/

[
ImplementedAs=TestImplements2Implementation, // Conflicts with default implements class name
NoInterfaceObject,
ImplementedAs=TestImplements2Implementation, // Conflicts with default implements class name
NoInterfaceObject, // Always used on target of 'implements'
RuntimeEnabled=Implements2FeatureName,
] interface TestImplements2 {
static attribute DOMString implements2StaticStringAttribute;
Expand Down
2 changes: 1 addition & 1 deletion Source/bindings/tests/idls/TestImplements3.idl
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

[
ImplementedInBaseClass, // Conflicts with default implements class name and [ImplementedAs]
NoInterfaceObject,
NoInterfaceObject, // Always used on target of 'implements'
] interface TestImplements3 {
attribute DOMString implements3StringAttribute;
static attribute DOMString implements3StaticStringAttribute;
Expand Down
90 changes: 59 additions & 31 deletions Source/bindings/tests/idls/TestInterface.idl
Original file line number Diff line number Diff line change
@@ -1,43 +1,71 @@
/*
* Copyright (C) 2010 Google Inc. All rights reserved.
* Copyright (C) 2013 Google Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
* modification, are permitted provided that the following conditions are
* met:
*
* 1. Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* 2. Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in the
* documentation and/or other materials provided with the distribution.
* 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of
* its contributors may be used to endorse or promote products derived
* from this software without specific prior written permission.
* * Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* * Redistributions in binary form must reproduce the above
* copyright notice, this list of conditions and the following disclaimer
* in the documentation and/or other materials provided with the
* distribution.
* * Neither the name of Google Inc. nor the names of its
* contributors may be used to endorse or promote products derived from
* this software without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
* EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
* WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
* DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
* DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
* (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
* LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
* ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
* THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
* A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
* OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/

// This IDL file is for testing the bindings code generator and for tracking
// changes in its ouput.
// Test for interface extended attributes and special operations.
// Also used as a target by TestObject

[
ActiveDOMObject,
Conditional=Condition1|Condition2,
ConstructorCallWith=ExecutionContext,
Constructor(DOMString str1, [Default=Undefined] optional DOMString str2),
RaisesException=Constructor,
SetWrapperReferenceTo(TestObject referencedName),
] interface TestInterface {
[ImplementedAs=getItem] getter (Node or NodeList) namedItem(DOMString name);
[Custom] setter Node (DOMString name, Node value);
Conditional=CONDITION,
Custom=LegacyCallAsFunction|ToV8,
DoNotCheckConstants,
ImplementedAs=TestInterfaceImplementation,
RuntimeEnabled=FeatureName,
SetWrapperReferenceTo(TestInterface referencedName),
StrictTypeChecking,
] interface TestInterface : TestInterfaceEmpty {
// members needed to test [ImplementedAs], as this affect attribute
// configuration and method configuration, and [StrictTypeChecking]
// constants also needed for [DoNotCheckConstants]
const unsigned long UNSIGNED_LONG = 0;
[Reflect=CONST_CPP] const short CONST_JAVASCRIPT = 1;

attribute TestInterface testInterfaceAttribute; // Self-referential interface type with [ImplementedAs]
attribute TestImplementedAsConstructor testImplementedAsConstructorAttribute;
static attribute DOMString staticStringAttribute;

void voidMethodTestInterfaceEmptyArg(TestInterfaceEmpty testInterfaceEmptyArg);
[PerWorldBindings] attribute DOMString perWorldBindingsStringAttribute;
[PerWorldBindings] void voidMethod();

// Anonymous indexed property operations
getter DOMString (unsigned long index);
setter DOMString (unsigned long index, DOMString value);
deleter boolean (unsigned long index);

// Anonymous named property operations
getter DOMString (DOMString name);
setter DOMString (DOMString name, DOMString value);
deleter boolean (DOMString name);
};

TestInterface implements TestImplements;
TestInterface implements TestImplements2;
TestInterface implements TestImplements3;
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2014 Google Inc. All rights reserved.
* Copyright (C) 2013 Google Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
Expand Down Expand Up @@ -28,7 +28,18 @@
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/

interface TestSpecialOperationsIdentifierRaisesException {
// This is for interface extended attributes that interact with another extended
// attribute, and thus both cannot be tested at once; and for special
// operations, which need a separate interface to test on.
// The more *minor* extended attribute should be put in this file.

[
Constructor, // Test interaction with [Custom=Wrap]
Custom=Wrap, // Conflicts with and [Custom=ToV8], respectively
DependentLifetime, // Covered by [ActiveDOMObject]
SetWrapperReferenceFrom=ownerNode, // Conflicts with [SetWrapperReferenceTo]
SpecialWrapFor=Interface1|Interface2, // Conflicts with [Custom=ToV8]
] interface TestInterface2 {
// Indexed property operations with an identifier
[RaisesException] getter TestInterfaceEmpty item(unsigned long index);
[RaisesException] setter DOMString setItem(unsigned long index, DOMString value);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,18 @@
*/

// This is for interface extended attributes that interact with another extended
// attribute, and thus both cannot be tested at once.
// attribute, and thus both cannot be tested at once; and for special
// operations, which need a separate interface to test on.
// The more *minor* extended attribute should be put in this file.

[
Custom=VisitDOMWrapper, // Conflict with [SetWrapperReferenceTo] and [SetWrapperReferenceFrom]
] interface TestInterfacePython3 {
Custom=VisitDOMWrapper, // Conflict with [SetWrapperReferenceTo] and [SetWrapperReferenceFrom]
] interface TestInterface3 {
[Custom] getter boolean (unsigned long index);
[Custom] setter boolean (unsigned long index, Node value);
[Custom] deleter boolean (unsigned long index);

[Custom=PropertyGetter|PropertyEnumerator|PropertyQuery] getter Node (DOMString name);
[Custom] setter Node (DOMString name, Node value);
[Custom] deleter boolean (DOMString name);
};
36 changes: 0 additions & 36 deletions Source/bindings/tests/idls/TestInterfaceDoNotCheckConstants.idl

This file was deleted.

53 changes: 0 additions & 53 deletions Source/bindings/tests/idls/TestInterfacePython.idl

This file was deleted.

0 comments on commit 8961684

Please sign in to comment.