Give an ability to WebKit to free statically allocated pointers before quit

• Component: WebKit Misc.
• Status: REOPENED ---

[Zoltan Herczeg]

For this enhancement we should keep in mind, that freeing statically allocated pointers takes time, which may delay the quit process. This may not be desired in standalone web browsers. However, sometimes it is good to free statically allocated pointers but this method should not have a performance impact when it is disabled.

[Zoltan Herczeg]

Created attachment 34058 [details]
First draft patch

[Zoltan Herczeg]

(In reply to comment #1)
> Created an attachment (id=34058) [details]
> First draft patch

This patch is intended to be a draft. I would like to hear your opinion about this solution:
  - two new files are added to wtf directory: StaticPtr.h and StaticPtr.cpp
  - enabling FREE_STATIC_PTRS allows to free the pointers in a reversed creation order
  - Although c++ supports automatic destructors, I think it would not be wise to free the pointers in random order.
  - StdLibExtras.h is overwritten to support these pointers
  - Since heap.destroy() must be called for JSGlobalData before delete, I created a new derived class, because template specialization created multiple definitions for StaticPtr<JSGlobalData>::free() (See JSDOMWindowBase.cpp -> commonJSGlobalData())
  - I am sure this patch should be cut to multiple parts. Do you have any suggestions for this?
  - Some destructors are only defined, but never implemented. Why? (~ImageLoadEventSender(), ~FontCache())
  - And an iteresting observation: some AtomicStrings are defined as const, and some are not, but all of them seems like fixed strings. Should we change all of their type to "const AtomicString"?

This patch works with Qt port nicely (after some fixes).

[Adam Treat]

I have grave doubts about the ability of this to work for all static pointers.  In particular I think the global static AtomicStrings in HTMLNames will cause problems.  Before seeing something like this go in I'd want to see it work for all the global statics and know that it wouldn't be a maintenance burden to keep it working when new ones are added or the order of them changed, etc.

[David Kilzer (:ddkilzer)]

I think enabling/disabling static pointer destruction should also be controllable by a runtime option as well (once enabled with the compile-time option).

[Eric Seidel (no email)]

if I remember correctly, Mac OS X's "leaks" tool, correctly ignores static pointer leaks.  Can't valgrind do the same?  with DEFINE_STATIC_LOCAL we even put them all into a custom section, which should make them super-easy to ignore. ;)

[Zoltan Herczeg]

(In reply to comment #4)

I think this would be an easy feature since WTF::StaticPtrBase::freeStaticPtrs() must be called from the code itself. With a runtime option we can easly prevent the call of this function.

[Zoltan Herczeg]

(In reply to comment #5)

The problem is with the transitive allocations. Valgrind does not track the use of 'this' pointers. Let A be a static object, and it allocates a B object, which also allocates a C object. A()->B()->C() In this case valgrind cannot see that B and C are belongs to the A object, and report them as memory errors. Perhaps it would be too time consuming to detect these situations.

[Darin Adler]

Comment on attachment 34058 [details]
First draft patch

I think it's a good idea to make our globals more consistent.

But probably not such a great idea to start what could be a massive project without a good idea how we'll keep test this and keep it working as each global is added.

Is the goal here to make memory leak debugging tools work better? Or is there value beyond that?

Could you write a document about how to use StaticPtr correctly? I'd like to review that to get an idea how we would make sure we were using it properly on various platforms. Would we consider it a coding style violation to make a global that doesn't use StaticPtr?

As part of this are you proposing that DEFINE_STATIC_LOCAL be the only legal way to make global variables?

I do not want to sign up everyone on the project to write a lot of code to deallocate every last object if this code isn't actually useful to someone on one of the ports. I need to hear more justification about why to take on all this additional complexity. Making all the global data structures practical to delete is an enormous task that this patch barely scratches the surface of.

An excellent economy in the project at the moment is that we don't try to write all the deallocation code for all the global objects.

>      wtf/RefCountedLeakCounter.cpp \
>      wtf/TypeTraits.cpp \
> +    wtf/StaticPtr.cpp \
>      wtf/unicode/CollatorDefault.cpp \

I believe this is supposed to be in alphabetic order, so it should come before TypeTraits.cpp.

> +#include "config.h"
> +
> +#include "StaticPtr.h"

Extra blank line here.

> +    StaticPtrBase() {
> +#if ENABLE(FREE_STATIC_PTRS)
> +        m_next = head();
> +        m_head = this;
> +#endif
> +    };

Extra semicolon here.

I think this would might read better if the inline implementation was outside the class definition since it has an #if in it. There's no reason we have to have the ifdef in here. But a better alternative would be to just leave out the constructor definition entirely when FREE_STATIC_PTRS is not enabled. If there are no constructor definitions then the class automatically will have an empty default constructor which is what we want.

> +    StaticPtr(PtrType ptr) : StaticPtrBase(), m_ptr(ptr) { };
> +    StaticPtr() : StaticPtrBase(), m_ptr(0) { };
> +    ~StaticPtr() { };

No need to explicitly initialize StaticPtrBase since there are no constructor arguments.

Extra semicolons here.

No need to explicitly declare the destructor -- this is the same as the compiler-generated constructor and it's much better style to not declare it.

> +    PtrType operator=(PtrType);

This seems like bad style. Since values stored in this pointer will be freed, I think we want this to use some explicit style like OwnPtr rather than plain assignment.

> +    ValueType& operator=(ValueType&);

This seems quite strange. Why do we need to allow assignment to the pointer of a non-const pointer of value type? This seems very dangerous, since the value type will later be destroyed.

> +    PtrType operator&();

> +    PtrType operator&() const;

Do we need this operator? What coding style is going to require this?

> +    // Condition operators
> +    bool operator!();

Should be const.

Please look at OwnPtr for some ideas on how to make this class work. This should be much closer to OwnPtr which has exactly the same semantic.

> +protected:

I believe these members should be private, not protected.

> +#if !PLATFORM(QT)
>      delete data;
> +#endif

This seems clearly wrong. Did you mean this to be a FREE_STATIC_PTRS ifdef?

> +    StaticGlobalData() : WTF::StaticPtr<JSGlobalData>() { };

This constructor is entirely unnecessary.

> +protected:

Should be private, not protected.

> +#if ENABLE(FREE_STATIC_PTRS)
> +    virtual void free() {
> +        m_ptr->heap.destroy();
> +        delete m_ptr;
> +    }
> +#endif

Incorrect brace style here.

> +ALWAYS_INLINE JSGlobalData* StaticGlobalData::operator=(JSGlobalData* value)
> +{
> +    m_ptr = value;
> +}

Seems poor to use plain assignment of a raw pointer for an operation that transfers ownership.

> -    return globalData;
> +    return &globalData;

This line of code seems wrong. You want a JSGlobalData*, not a StaticGlobalData*. This is a poor use of operator overloading.

review- for now based on the specific problems with the patch, but I do also have project-direction concerns here, although I am not using reviewer power to try to enforce those.

[Zoltan Herczeg]

(In reply to comment #8)
> (From update of attachment 34058 [details])
> I think it's a good idea to make our globals more consistent.

Thank you!

> But probably not such a great idea to start what could be a massive project
> without a good idea how we'll keep test this and keep it working as each global
> is added.

Totally agree. It is not my custom to submit a patch in such draft phase, but I feel this project is more about finding a way to define globals which everyone _likes_, than the actual code itself. I know this is risky, may never be a successful project, but it is worth a try, isn't it? :)

I will fix typos and answer to all your questions tomorrow.

Once again, thank you for your detailed feedback!

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> (From update of attachment 34058 [details])
> I do not want to sign up everyone on the project to write a lot of code to
> deallocate every last object if this code isn't actually useful to someone on
> one of the ports. I need to hear more justification about why to take on all
> this additional complexity. Making all the global data structures practical to
> delete is an enormous task that this patch barely scratches the surface of.

Being able to "reset" WebCore to its pre-cached-local-statics state would be useful for the iPhone OS port.  We don't care about tearing down objects when exiting a process, but being able to clear out the cached static variables on-demand would be useful.

[Zoltan Herczeg]

Created attachment 34131 [details]
Second draft patch

[Zoltan Herczeg]

Unfortunately there are several ways to define global variables, I don't think it is possible to merge them into a single preprocessor define:
 - using DEFINE_STATIC_LOCAL
   * creates static global variables inside a function
   * the object is created before the first call of the function
 - using DEFINE_GLOBAL (WebCore/platform/StaticConstructors.h)
   * these variables cannot be freed, since they are allocated on the heap
   * workaround: wrapper classes
     Any better idea?
   * fortunately it is a rarely used directive (only two classes: AtomicString and QualifiedName)
 - Plain global variables
   [static] class* [class_name::]variable;
   * I will try to use a static alayzer tool to find these objects
   * my propose:
     use '[static] WTF::StaticPtr<class> [class_name::]variable;' instead of them in the future

WTF::StaticPtr similar to OwnPtr, except:
  - only one object can be assigned to it. The object can be passed to the constructor, or assigned later.

> Is the goal here to make memory leak debugging tools work better? Or is there
> value beyond that?

Ok, you win here :) tonikitoo and ddkilzer mentioned they could use this feature. I also plan to use it for embedded devices, but first I would like to know whether it is possible to do it. But right know they are just plans, nothing more.

> Could you write a document about how to use StaticPtr correctly? I'd like to
> review that to get an idea how we would make sure we were using it properly on
> various platforms. Would we consider it a coding style violation to make a
> global that doesn't use StaticPtr?

"WTF::StaticPtr<class>" would be a substitution of "class*", and it would mimic the default C++ pointer behaviour as possible (with operator overloading). I hope "class&" definitions are rare except for DEFINE_STATIC_LOCAL.

> As part of this are you proposing that DEFINE_STATIC_LOCAL be the only legal
> way to make global variables?

No. DEFINE_STATIC_LOCAL has a special purpose, and looks like not cover all global variables.

> I do not want to sign up everyone on the project to write a lot of code to
> deallocate every last object if this code isn't actually useful to someone on
> one of the ports. I need to hear more justification about why to take on all
> this additional complexity. Making all the global data structures practical to
> delete is an enormous task that this patch barely scratches the surface of.

Well I think a global policy can be:
  - either do not write destructors (someone else can do later)
  - or write a working, and tested constructor.

> > +#if !PLATFORM(QT)
> >      delete data;
> > +#endif

This delete cause a segmentation fault in Qt. It has not appeared until now, since this destructor is never called. (Although this code may have worked, when the destructor was implemented, but it was not covered by regression tests, and has gone wrong eventually).

I think if we write a destructor, we have to maintain it as the other codes, and should cover them in regression tests. This may be a good aim for this project.

I have fixed the typos and other things (the looks much better now), except:

> > +    PtrType operator=(PtrType);
> 
> This seems like bad style. Since values stored in this pointer will be freed, I
> think we want this to use some explicit style like OwnPtr rather than plain
> assignment.

Could you show me an example here?

Anyway, a plain QtLauncher now frees 761 objects (without crash). After some browsing it grows to 812.

[Adam Treat]

(In reply to comment #12)
>  - using DEFINE_GLOBAL (WebCore/platform/StaticConstructors.h)
>    * these variables cannot be freed, since they are allocated on the heap
>    * workaround: wrapper classes
>      Any better idea?
>    * fortunately it is a rarely used directive (only two classes: AtomicString
> and QualifiedName)

And those classes have lots of instances.  In fact, I'd wager the majority of static globals you'll find in WebCore are of this type.  What's more, have fun trying to destruct them in the right order.

One other concern I don't think you've considered is what happens when these global statics are allocated on the heap with placement new?  This occurs right now in WebCore if you look carefully :)  With placement new allocated global statics you'll have to explicitly call the destructor.  Not sure how you are going to automate that one.

[Zoltan Herczeg]

> And those classes have lots of instances.  In fact, I'd wager the majority of
> static globals you'll find in WebCore are of this type.  What's more, have fun
> trying to destruct them in the right order.
> 
> One other concern I don't think you've considered is what happens when these
> global statics are allocated on the heap with placement new?  This occurs right
> now in WebCore if you look carefully :)  With placement new allocated global
> statics you'll have to explicitly call the destructor.  Not sure how you are
> going to automate that one.

In may patch a specialized wrapper class inherited from StaticPtrBase solves this problem (in WebCore at least). See GlobalAtomicString and GlobalQualifiedName. The placement news are replaced by a macro call (INIT_GLOBAL) which has a different behaviour depending on ENABLE(FREE_STATIC_PTRS). (I belive placement news are ugly, and now it looks much better)

Adam, if you really want I can stop this task. I don't want to force anything if you don't like it.

[Adam Treat]

(In reply to comment #14)
> > And those classes have lots of instances.  In fact, I'd wager the majority of
> > static globals you'll find in WebCore are of this type.  What's more, have fun
> > trying to destruct them in the right order.
> > 
> > One other concern I don't think you've considered is what happens when these
> > global statics are allocated on the heap with placement new?  This occurs right
> > now in WebCore if you look carefully :)  With placement new allocated global
> > statics you'll have to explicitly call the destructor.  Not sure how you are
> > going to automate that one.
> 
> In may patch a specialized wrapper class inherited from StaticPtrBase solves
> this problem (in WebCore at least). See GlobalAtomicString and
> GlobalQualifiedName. The placement news are replaced by a macro call
> (INIT_GLOBAL) which has a different behaviour depending on
> ENABLE(FREE_STATIC_PTRS). (I belive placement news are ugly, and now it looks
> much better)

I see your second patch is special casing them, but it is not deleting them as you are not explicitly calling the destructor you are just deleting them as normal.
 
> Adam, if you really want I can stop this task. I don't want to force anything
> if you don't like it.

No, feel free to continue I just have doubts that this method of dealing with it will be robust enough to deal with the stated goal without encuring a lot of new requirements.

[Zoltan Herczeg]

Created attachment 34265 [details]
Third patch

The whole thing is in much better shape. I hope you will like it.

[Eric Seidel (no email)]

In general I don't really like this idea.  Ripping things down during quit is only useful for leak debugging.  On Mac OS X we've not had this trouble with our leak debugging tools to my knowledge.  I'm sad that we would have to jump through hoops for valgrind.  But maybe I'm missing something obvious.

Darin Adler is likely your best bet for a reviewer.  He's still the best source for approval on such large sweeping changes.  The man has more c++-fu in his pinky finger than all of me does. ;)

[Zoltan Herczeg]

Created attachment 34549 [details]
Fourth patch

Actually WebCore is much better written than I expected, so uniforming the constant pointer handling is not that hard.

[Zoltan Herczeg]

After starting and quiting from QtLauncher, the leak reduced from:

==18154== LEAK SUMMARY:
==18154==    definitely lost: 66,632 bytes in 496 blocks.
==18154==      possibly lost: 25,536 bytes in 7 blocks.
==18154==    still reachable: 241,690 bytes in 5,595 blocks.
==18154==         suppressed: 0 bytes in 0 blocks.

Number of loss records: 295

To:

==6905== LEAK SUMMARY:
==6905==    definitely lost: 344 bytes in 2 blocks.
==6905==    indirectly lost: 20 bytes in 1 blocks.
==6905==      possibly lost: 744 bytes in 3 blocks.
==6905==    still reachable: 117,418 bytes in 2,859 blocks.
==6905==         suppressed: 0 bytes in 0 blocks.

Number of loss records: 131

And 50k from this space is a debug structure used by StructureID-s.

[Yong Li]

Is there a specified webkit name pattern for global variables? In this patch, I've seen both "foo" (same as local variables) and "gFoo". I use "g_Foo" in my code, but Eric says it's wrong. So what pattern should be used for global variables?

[Yong Li]

(In reply to comment #20)
> Is there a specified webkit name pattern for global variables? In this patch,
> I've seen both "foo" (same as local variables) and "gFoo". I use "g_Foo" in my
> code, but Eric says it's wrong. So what pattern should be used for global
> variables?

correction: should be "g_foo"

[Darin Adler]

Comment on attachment 34549 [details]
Fourth patch

> +#include "wtf/StaticPtr.h"

Includes of wtf from WebCore use angle brackets. <wtf/StaticPtr.h>, not "wtf/StaticPtr.h".

> +static WTF::StaticPtr<JSGlobalData> sharedInstancePtr;

This won't work with the restrictions we need to work as a Mac OS X framework. We can't have any static destructors in our Mac OS X system frameworks as a matter of policy to make process exit fast. No objects with destructors as globals or statics. That's the reason for the DEFINE_STATIC_LOCAL macro.

A major reason this patch is unacceptable is that this creates many global objects with destructors.

I didn't review further because that's a major problem with the current version.

[Zoltan Herczeg]

> This won't work with the restrictions we need to work as a Mac OS X framework.
> We can't have any static destructors in our Mac OS X system frameworks as a
> matter of policy to make process exit fast. No objects with destructors as
> globals or statics. That's the reason for the DEFINE_STATIC_LOCAL macro.

I have realized why DEFINE_STATIC_LOCAL (probably DEFINE_GLOBAL is even better example, which use placement new to hide the destructor) defined, so the destructor of WTF::StaticPtr is empty of course, and never intended to do anything...

[Darin Adler]

(In reply to comment #23)
> I have realized why DEFINE_STATIC_LOCAL (probably DEFINE_GLOBAL is even better
> example, which use placement new to hide the destructor) defined, so the
> destructor of WTF::StaticPtr is empty of course, and never intended to do
> anything...

Please check and see what the compiler generates for this before giving a patch that deploys it in tons of places. I'm pretty sure that an empty virtual function will indeed generate a static destructor, although a pointless empty one. There's a good chance it will even change the virtual pointer on the object.

Anyway, if the technique works that's fine but it's pointless to review a patch to deploy the technique before you've tested it!

[Yong Li]

(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=34058) [details] [details]
> > First draft patch
> 
> This patch is intended to be a draft. I would like to hear your opinion about
> this solution:
>   - two new files are added to wtf directory: StaticPtr.h and StaticPtr.cpp
>   - enabling FREE_STATIC_PTRS allows to free the pointers in a reversed
> creation order
>   - Although c++ supports automatic destructors, I think it would not be wise
> to free the pointers in random order.

Why not just let ~StaticPtr() to free memory? Static objects should be destructed in the reverse order of construction, as when a static object is constructed, atexit will be pushed to onexit queue. Also, you can always destruct all static objects by calling _cexit() explicitly. The only benefit of using a chain I can see is that it destructs all object first, and then frees all the memory, so that when a destructor accesses to another destructed static object, it won't result segment fault...

[Yong Li]

(In reply to comment #25)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > Created an attachment (id=34058) [details] [details] [details]
> > > First draft patch
> > 
> > This patch is intended to be a draft. I would like to hear your opinion about
> > this solution:
> >   - two new files are added to wtf directory: StaticPtr.h and StaticPtr.cpp
> >   - enabling FREE_STATIC_PTRS allows to free the pointers in a reversed
> > creation order
> >   - Although c++ supports automatic destructors, I think it would not be wise
> > to free the pointers in random order.
> 
> Why not just let ~StaticPtr() to free memory? Static objects should be
> destructed in the reverse order of construction, as when a static object is
> constructed, atexit will be pushed to onexit queue. Also, you can always
> destruct all static objects by calling _cexit() explicitly. The only benefit of
> using a chain I can see is that it destructs all object first, and then frees
> all the memory, so that when a destructor accesses to another destructed static
> object, it won't result segment fault...

correction: atexit pushes the destructor to the onexit stack

[Balazs Kelemen]

A branch had been set up on gitorius where I tried to extend the patch(es) for all statics: [e-mail]/~balazs/qtwebkit/balazs-clone/commits/free-static-ptrs. Actually it is tested only in interpreter mode (DEFINES='ENABLE_JIT=0 ENABLE_YARR_JIT=0' at the end of the build-webkit ... command line) of JSC because I had some problems with JIT.

[Zoltan Herczeg]

Created attachment 38848 [details]
Fifth patch

Sorry for my long silence, I was away on Holiday. From the start of this week I am back again and has rewritten most of the core parts of the patch. Global pointers are changed to structures (these structures are allocated on .bss (according to objdump). They are simple C structures -> no constructors or destructors are called (tested by gdb)). Now every structure has an own separate file ()

The new files are:
   - contains a C struct
 create mode 100644 JavaScriptCore/wtf/GlobalPtr.h
   - contains a C++ class
 create mode 100644 JavaScriptCore/wtf/LocalStaticArrayPtr.h
   - contains a C++ class
 create mode 100644 JavaScriptCore/wtf/LocalStaticPtr.h
   - freeing static variables
 create mode 100644 JavaScriptCore/wtf/StaticPtrBase.cpp
   - contains a C struct
 create mode 100644 JavaScriptCore/wtf/StaticPtrBase.h

There is no inheritance, since a struct is converted to class if inheritance applied (was strange for me, but it is true). Instead StaticPtrBase must be the first member of its descendants. The descendants must not contain any virtual method (to avoid vptr).

[Zoltan Herczeg]

Darin, could you take a look at the patch, please. I had a qestion on webkit-dev about whether should I unifying the names of global variables along this work. What is your opinion?

[Darin Adler]

Zoltan, I understand your enthusiasm for this project and I am willing to review when I have the time. But this patch is not a high priority for me.

I would like this to be done right if it is done, I still don't think it's worth doing.

Who exactly is planning to take advantage of this change, and for what? Is the goal making leak-detection tools work better? Is someone planning to unload the WebKit library without exiting the process it's hosted in?

You say "sometimes it is good to free statically allocated pointers". When?

[Darin Adler]

(In reply to comment #29)
> I had a qestion on
> webkit-dev about whether should I unifying the names of global variables along
> this work. What is your opinion?

I think it's best to do renames separately. I'm always tempted to do such things together and it tends to make patches harder to review, and if there is a problem later, more conflicts and difficulty rolling a change out.

[Yong Li]

(In reply to comment #30)
> 
> You say "sometimes it is good to free statically allocated pointers". When?

One case is that webkit is used as a dll, and the dll can be loaded and unloaded multiple times. Those global pointers will leak in this case.

But I guess an OwnPtr or something should be good for this purpose.

The dtors will be called when the dll is unloaded. Also there is a c_exit or _cexit in MSVC to explicitly call all dtors for global/static objects. When constructing a global object, the compiler will add the dtor to the exit stack with atexit. So the order of destruction is reverse order of construction. and explicitly calling c_exit() also clears the stack, so no need to worry about the problem of calling dtor twice. Is there a compiler that doesn't guarantee this?

ENABLE(FREE_STATIC_PTRS) is good for not affecting people who don't want to use it. But GlobalPtr must be freed by explicit call to deletePtr(). That's not an easy life. Considering a coding guideline that requires using same macros for global objects.

What are the goals?

1. construct and destruct global objects in safe order

2. ability to free all memory allocated by global pointers

3. ability to leave those global pointers as leaks.

We can just define a GlobalPtr same as OwnPtr, except there's a compile-time switch for deleting the object in destructor or not. That would be simple but enough for most ports I guess.

[Darin Adler]

(In reply to comment #32)
> One case is that webkit is used as a dll, and the dll can be loaded and
> unloaded multiple times.

Is there someone who wants to use WebKit this way? Is the global variable issue the only issue preventing this?

I think the project to make it so you can load and unload WebKit multiple times is a pretty big project. And we should not do it unless we have to.

[Yong Li]

(In reply to comment #33)
> (In reply to comment #32)
> > One case is that webkit is used as a dll, and the dll can be loaded and
> > unloaded multiple times.
> 
> Is there someone who wants to use WebKit this way? 

Yes. for example, webkit is used to render html page in other applications, like a COM object provider, or other ways.

> Is the global variable issue the only issue preventing this?
> I think the project to make it so you can load and unload WebKit multiple times
> is a pretty big project. And we should not do it unless we have to.

There may be other resources to consider, but those static global pointers are hard to be explicitly freed outside of WebCore. Loading/unloading webkit dll multiple times is fine except the leaks.

[Zoltan Herczeg]

(In reply to comment #30)
> Zoltan, I understand your enthusiasm for this project and I am willing to
> review when I have the time. But this patch is not a high priority for me.
> 
> I would like this to be done right if it is done, I still don't think it's
> worth doing.
> 
> Who exactly is planning to take advantage of this change, and for what? Is the
> goal making leak-detection tools work better? Is someone planning to unload the
> WebKit library without exiting the process it's hosted in?
> 
> You say "sometimes it is good to free statically allocated pointers". When?

Perhaps this patch will be destined as an experimental solution. The big qestion is: whether it is possible to make WebKit to an unloadable dll / shared object. The answer is unknown at this moment, since noone tried it before, so noone will start to consider this as an option because they feel this is a too risky project. We just close the way before an interesting feature without trying it.

First, I try to sort all global variables into groups. How globals are declared and used in WebKit. Is there a redundant form, or all of them are necessary (now I have only 3 groups).

Second, are the globals handled correctly. Some globals are change their value, and we must make sure (in debug mode) their previous value is freed.

Right now I found only one exception to the reversed free order: WebCore::pageCache() is allocated too early in QtLauncher (using setMaximumPagesInCache), and its destructor frees a timer object. The calling of timer::stop() causes segmentaion fault at that time.

I don't insist on the current form of global handling. If all globals are declared as templates, anyone can change their behaviour (and make GlobalPtr to a class if someone prefers that way).

The patch really helps to valgrind: a simple QtLauncher start-exit process left around 300k leak, and this is reduced to 100k. The number of loss records decreased from 400 to 100. It is much easier to find real leaks from that 100 loss records (especially because the remaing loss records are belongs to font config and Qt internals).

There is one really good news: the work started 1.5 monts ago, and I had no trouble upgrading it to the most recent version so far. Looks like maintaining of this patch is not a hard work (hopefully).

[Zoltan Herczeg]

Created attachment 39334 [details]
Sixth patch

Just a regular update. Many new pointers are freed now.

[Darin Adler]

Comment on attachment 39334 [details]
Sixth patch

I really dislike the direction this is going. Adding many new classes and lots of complexity. And lots of code that won't compile in most platforms, so will cause platform-specific build failures.

I don't think this is worth it.

> +#include "wtf/GlobalPtr.h"

Includes of wtf should be:

    #include <wtf/GlobalPtr.h>

I think GlobalPtr should probably be named GlobalOwnPtr and have an interface as close as possible to the OwnPtr class, since it's a sort of hybrid between OwnPtr and a raw pointer.

> +WTF::GlobalPtr<JSGlobalData> sharedInstancePtr;

Items in the WTF namespace are supposed to have "using WTF::GlobalPtr" in the header and not be qualified by WTF explicitly in code.

I don't understand why it's better to move the actual pointer outside of a function. It still seems a good programming style to scope a global variable inside a function when that's possible.

> +#include "wtf/FastMalloc.h"
> +#include "wtf/StaticPtrBase.h"

Need to use angle brackets for these includes.

> +// All HashTables are static and generated by a script (see the comment in the header file)

Need a period at the end of this sentence.

> +struct HashTableDeleter {
> +public:

No need for "public" since this is a struct. A struct where everything is public should omit the public: part.

> +    WTF::StaticPtrBase m_base;

No WTF:: prefix.

> +    HashTable* m_hashTable;
> +
> +    static void free(void* ptr)
> +    {
> +        reinterpret_cast<HashTableDeleter*>(ptr)->m_hashTable->deleteTable();

This should be a static_cast, not reinterpret_cast.

> +#if ENABLE(FREE_STATIC_PTRS)
> +    HashTableDeleter* deleter = reinterpret_cast<HashTableDeleter*>(fastMalloc(sizeof(HashTableDeleter)));

This should be a static_cast, not a reinterpret_cast.

Why use fastMalloc here and not new?

> +// The classes here are defined as structs to avoid
> +// the call of C++ constructors and destructors

This comment and technique is incorrect. Using the keyword "struct" instead of "class" has no effect on the presence of C++ constructors and destructors.

There are only two differences between class and struct:

    1) Default visibility is public for struct, private for class.

    2) Visual Studio treats struct and class as distinct for type identity and name mangling. This goes against the C++ standard, but requires that we be consistent if we want compatibility with that compiler.

Otherwise, they are identical and struct vs. class has no effect on whether something has a non-trivial constructor and destructor.

> +    // Has NO C++ constructor

The terminology here is strange. Why say "C++" when all this code is C++?

The comments in this file are not clarifying what this class is for. They need to say that it's intended to be used for global data. And to satisfy our requirement that there be no global destructors in the WebKit projects if FREE_STATIC_PTRS is not set, they must have no non-trivial destructor. And to satisfy our requirement that there be no global constructors in the WebKit projects they must have no non-trivial constructor.

Some of that needs to be said.

> +    void set(PtrType);
> +    void setPtr(PtrType);

I have no idea how set differs from setPtr. Does not seem to be good design to have both, but perhaps there is some real difference here. If so we need to use a more descriptive name. For example, in other places we use terms like "adopt". Somehow I guess setPtr means "take this pointer value, but not ownership". And if so, it seems very strange design to have a pointer that sometimes owns what it points to, and other times does not. Seems like overkill to have that. If it's not going to own something, then we can use a raw pointer.

> +    void operator=(PtrType);

And also have an = operator. Why have all three of these?

> +    void deletePtr();

What is this for?

> +#if ENABLE(FREE_STATIC_PTRS)
> +    StaticPtrBase m_base;
> +#endif

This makes every global pointer 8 bytes. Is this really the best we can do?

> +        delete reinterpret_cast<GlobalPtr<T>*>(ptr)->get();

This should be a static_cast, not reinterpret_cast.

> +    void setFree(StaticPtrBaseFree func)

The type name StaticPtrBaseFree is not so good, because "free" is a verb or adjective, not a noun. Types normally should be nouns.

Instead of "func" please use "function". There's no need to abbreviate. Except in certain cases where something needs to be

> +    // Detect memory leaks
> +    ASSERT(!m_ptr || (m_ptr == value));

The comment here is confusing. How does this comment "detect memory leaks"? I think it's strange to have this function allow you to set the same value multiple times, given that the set operation is really "give ownership". Why do we need that flexibility? I think the set function should probably be named "adopt" if it is taking ownership.

> +template<typename T>
> +ALWAYS_INLINE void GlobalPtr<T>::operator=(PtrType value)

Do we really need this? I think assignment is confusing when it transfers ownership of a raw pointer, so it would be best to leave this out unless it's really needed.

> +// Wrapper class for arrays, which assigned to a static variable
> +// By default, it behaves like a simple C++ array without performance overhead

These comments need periods.

The comment leaves out what really matters, which is what this class is for! It says that by default it works as a simple array, but what does it do when not default?

> +        delete[] reinterpret_cast<LocalStaticArrayPtr<T>*>(ptr)->m_ptr;

This should be static_cast.

> +    while (current) {
> +        // Some pointers (i.e: HashTableDeleter) frees themself

"Some pointers free themselves." would be correct grammar.

The Latin abbreviation "i.e." means "in other words". I think you mean "for example" here and that should be written either as "for example" or as "e.g." if you want to use the Latin abbreviation.

The comment is *almost* helpful, but is just a bit too obscure. The comment needs to be more explicit and say clearly that we follow the next pointer before calling free because in some cases the free function will delete the item including the next pointer.

> +        ptr->m_free(reinterpret_cast<void*>(ptr));

There is no need for this cast at all.

> +    // Perhaps a double linked list may be suitable if remove() called frequently

This is a confusing comment, because it does not say what the tradeoff is. Why aren't we using a doubly-linked list now? I suggest we not even include the remove() function unless we need it. And if we need it we should have the doubly-linked list from the start, unless there's a clear reason we should not.

> +    StaticPtrBase* current = head();
> +    if (current == this) {
> +        m_head = current->next();
> +        return;
> +    }
> +
> +    while (current->next() != this) {
> +        current = current->next();
> +        ASSERT(current);
> +    }
> +    current->m_next = next();
> +}

It's a little strange to use next() to get the pointer and then m_next to set it. There's no abstraction there, just slight confusion, by having some of each. There are cleaner ways to write the linked list algorithms that require fewer special cases and don't fetch current->next() multiple times, but this is probably OK as-is.

> +    // Methods
> +    void append();
> +    void remove();

This comment is not good. First of all, "methods" is not a C++ term, and second, what does that comment add?

> +    static StaticPtrBase* m_head;
> +    StaticPtrBase* m_next;
> +    StaticPtrBaseFree m_free;
> +
> +    static inline StaticPtrBase* head() { return m_head; }
> +    inline StaticPtrBase* next() { return m_next; }

The use of inline here does no good -- functions defined inside a C++ class are always treated as if they had "inline" specified, so there's no point in saying it explicitly.

It doesn't make sense to have both public data members and public function members that get at that same data. Either the data should be private, or the functions should be omitted.

> +#if !PLATFORM(QT)
>      delete data;
> +#endif

Why is Qt a special case here? This is very confusing!

> --- a/WebCore/WebCore.pro
> +++ b/WebCore/WebCore.pro
> @@ -1044,6 +1044,7 @@ SOURCES += \
>      html/HTMLViewSourceDocument.cpp \
>      html/ImageData.cpp \
>      html/PreloadScanner.cpp \
> +    html/TimeRanges.cpp \
>      html/ValidityState.cpp \
>      inspector/ConsoleMessage.cpp \
>      inspector/InspectorBackend.cpp \
> @@ -2629,7 +2630,6 @@ contains(DEFINES, ENABLE_VIDEO=1) {
>          html/HTMLMediaElement.cpp \
>          html/HTMLSourceElement.cpp \
>          html/HTMLVideoElement.cpp \
> -        html/TimeRanges.cpp \
>          platform/graphics/MediaPlayer.cpp \
>          rendering/MediaControlElements.cpp \
>          rendering/RenderVideo.cpp \

Can you make the above change in a separate check-in first?

> +#if !ENABLE(FREE_STATIC_PTRS)
>      ~IdentifierRep()
>      {
>          // IdentifierReps should never be deleted.
>          ASSERT_NOT_REACHED();
>      }
> +#endif

This is a good example of something that's probably wrong. I think there is storage to be freed here. Simply changing things so it compiles means that we have a storage leak in the FREE_STATIC_PTRS case.

This patch is way too big, doing too much at once. It is easy to miss mistakes like this in a patch of this size.

>      static PassRefPtr<CSSInitialValue> createExplicit()
>      {
> -        static CSSInitialValue* explicitValue = new CSSInitialValue(false);
> -        return explicitValue;
> +        DEFINE_STATIC_LOCAL(CSSInitialValue, explicitValue, (false));
> +        return &explicitValue;
>      }
>      static PassRefPtr<CSSInitialValue> createImplicit()
>      {
> -        static CSSInitialValue* explicitValue = new CSSInitialValue(true);
> -        return explicitValue;
> +        DEFINE_STATIC_LOCAL(CSSInitialValue, explicitValue, (true));
> +        return &explicitValue;
>      }

These kinds of changes should go in first, in a separate patch.

> -        delete defaultStyle;
> -        delete simpleDefaultStyleSheet;
> +        defaultStyle.deletePtr();
> +        simpleDefaultStyleSheet.deletePtr();
>          defaultStyle = new CSSRuleSet;
>          simpleDefaultStyleSheet = 0;

This is a bad idiom. The global pointer is a variation on OwnPtr, and the right idiom there is to delete things automatically when assigning a new value. Having an explicit deletePtr makes it too easy to use it wrong.

It's extremely important to have a patch that creates  the mechanism separate from a patch that deploys it in so many places. By making a single large patch you're making it very hard to review, and there are lots of things you will have to change now based on my feedback.

I can't read the rest of this right now.

[Alexey Proskuryakov]

*** Bug 30408 has been marked as a duplicate of this bug. ***

[Carol Szabo]

I believe that the value of being able to clean up static pointers goes well beyond reporting memory leaks.
There are operating systems that run on embedded devices that use WebKit that run all applications in one process space. They unload the WebKit library, but any unfreed memory is leaked until the device is reset. On these devices snapshots of WebKit have been customized to free these pointers. It would be nice to have this code merged to the trunk.
There are applications that host various WebKit based WebWidgets that are created and destroyed without the application quitting. For this class of applications deleting the static pointers on library unload is important.
Next, there is the case described in an earlier comment of resetting the state of WebKit.
Also, I believe that the proposal suggested earlier by somebody to run all destructors before actually freeing the static allocations should take care of circular references and delete order despite the fact that in a well designed class architecture delete order of global objects should be irrelevant.
I am not saying that the default Mac build or even any platform's default build should be affected in any way by these changes, but one should have the option to make a build that supports a clean uninitialization.

Whenever something is built without provisions for clean tearing down sooner or later problems will appear (think nuclear plants and nuclear waste).

[Carol Szabo]

Created attachment 41886 [details]
Proposed Partial Patch

This topic appears to be pretty controversial. It appears that there is not much value at least for the Mac version of WebKit in making WebKit clean up its singletons before it being unloaded.
Since a large change appears to be expensive in terms of development and review time, and risky enough to be found unjustifiable for Apple, I propose this small safe change, that does not solve the whole problem, but can constitute a first step towards solving the problem.
My final goal is to have global resources (such as cashes, custom memory management maps, default stylesheets, etc.) implemented in such a way that after the last WebKit page, frame, document, script object, worker thread, etc. has been deleted, these resources can be freed in any order, thus they can be implemented as global variables, or via global smart pointers, etc. if one wanted to.
I plan to provide macros for declaring and defining these global resources.
I also plan that by default these macros shall expand to pretty much the same code as it is now used for the resources such that there is no impact on performance.
In the absence of any other value, this current patch provides:
1. No impact on performance if the user does not redefine the DEFINE_STATIC_LOCAL macro.
2. A slightly smaller, more consistent yet more configurable WebKit source code which benefits future maintainability and usability.

I believe that on the basis of the above benefits alone, my patch should be accepted, regardless of whether I manage to find a solution to the underlying problem of singleton cleanup.

[Darin Adler]

Comment on attachment 41886 [details]
Proposed Partial Patch

It seems fine to use DEFINE_STATIC_LOCAL in one more place. And since that's all the patch does, we don't have to have any deep debate about it.

It also seems OK to allow a version of DEFINE_STATIC_LOCAL defined elsewhere to override the version in StdLibExtras.h, but I don't see any reason the two need to be done in a single patch.

> +#ifndef DEFINE_STATIC_LOCAL
>  #if COMPILER(GCC) && defined(__APPLE_CC__) && __GNUC__ == 4 && __GNUC_MINOR__ == 0 && __GNUC_PATCHLEVEL__ == 1
>  #define DEFINE_STATIC_LOCAL(type, name, arguments) \
>      static type* name##Ptr = new type arguments; \
> @@ -40,7 +41,7 @@
>  #define DEFINE_STATIC_LOCAL(type, name, arguments) \
>      static type& name = *new type arguments
>  #endif
> -
> +#endif
>  // OBJECT_OFFSETOF: Like the C++ offsetof macro, but you can use it with classes.
>  // The magic number 0x4000 is insignificant. We use it to avoid using NULL, since
>  // NULL can cause compiler problems, especially in cases of multiple inheritance.

You should move the #endif here so that there's still a blank line before OBJECT_OFFSETOF.

> +        * platform/ThreadGlobalData.cpp:
> +        (WebCore::threadGlobalData):
> +        Allocated threadGlobalData statically, not on heap such that it
> +        will be destroyed when the library is unloaded.

I do not think the comment here is clear. My comment would be something more like, "Use DEFINE_STATIC_LOCAL macro instead of doing a one-offer version here. Helpful because we are using DEFINE_STATIC_LOCAL to experiment with unloading the library and deallocating memory".

I think these patches are OK, but the change log should talk about what they do rather than saying these give WebKit ability to free memory when unloading. That's a big project that this patch is only a tiny part of.

review- because of the minor problem with #endif and because the change log could be a lot clearer, but the patch seems fine otherwise

[Carol Szabo]

Created attachment 41972 [details]
Allowing for override of DEFINE_STATIC_LOCAL

Per Darin's comment, I split my patch in 2 and I placed the appropriate comment in the ChangeLog.
I apologize for my carelessness in the previous patch proposal which had unrelated comments in the ChangeLog as a result of reusing the log from a previous more substantial attempt.

[Eric Seidel (no email)]

Comment on attachment 41972 [details]
Allowing for override of DEFINE_STATIC_LOCAL

So can DEFINE_STATIC_LOCAL still eventually be removed once we no longer support this broken version of Apple's GCC?  Or does this new usage make us forever dependent on this macro?

[Darin Adler]

(In reply to comment #43)
> So can DEFINE_STATIC_LOCAL still eventually be removed once we no longer
> support this broken version of Apple's GCC?  Or does this new usage make us
> forever dependent on this macro?

I don't think removing DEFINE_STATIC_LOCAL is a good idea, regardless of this new usage. It's true that we use it to work around a GCC bug. But we also use it to make indicate the idiom of using a local variable in a way that avoids static destructors. It's better to that than to handwrite the idiom each time.

We might want to rename it, but it seems unlikely we'd want to remove it.

[WebKit Commit Bot]

Comment on attachment 41972 [details]
Allowing for override of DEFINE_STATIC_LOCAL

Clearing flags on attachment: 41972

Committed r50171: <http://trac.webkit.org/changeset/50171>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antonio Gomes]

i think carol's landed patch is a small part of the big thing here (?) or zoltan's implementation showed up itself as unfeasible and was "dropped" (and i missed it in the bug comments) ?

commitbot landed and closed the bug by accident then if so  (?)

ps: sorry for bug spamming

[Alexey Proskuryakov]

Definitely so, reopening.

[Eric Seidel (no email)]

We try to use one change per bug.  It would be better to create a new "meta bug" and if there are multipel changes, do those in individual bugs blockign the meta bug.

[Alexey Proskuryakov]

There is a lot of discussion in this bug, better not start over, in my opinion.