Turn on adoptRef assertion for RefCounted

• Component: New Bugs
• Status: RESOLVED FIXED

[Darin Adler]

Turn on adoptRef assertion for RefCounted

[Darin Adler]

Created attachment 60415 [details]
Patch

[Adam Barth]

Comment on attachment 60415 [details]
Patch

A bunch of minor comments below.  Really glad to see this patch!  Thanks for picking up this project and running with it.

WebCore/html/FileStreamProxy.cpp:60
 +      proxy->ref();
This seems like a bad design.  The way we solved this problem in Chrome is by having specialized Task objects that understood how to ref/deref the objects that they operated upon.  It took a few iterations of the design to get it right, but we got rid of almost all the instances of this (error-prone) pattern.

WebCore/page/EventSource.cpp:71
 +  PassRefPtr<EventSource> EventSource::create(const String& url, ScriptExecutionContext* context, ExceptionCode& ec)
This change is slightly complex, but looks like a nice win in terms of readability.

WebCore/page/EventSource.h: 
 +          EventSource(const String& url, ScriptExecutionContext* context, ExceptionCode& ec);
Yeah, having the ec in the constructor seems like nonsense semantically.  Constructors that can fail don't make much sense.

WebCore/storage/StorageAreaSync.h:44
 +          static PassRefPtr<StorageAreaSync> create(PassRefPtr<StorageSyncManager>, PassRefPtr<StorageAreaImpl>, const String& databaseIdentifier);
We should improve the linter to find Strings passed by value.

WebCore/workers/SharedWorker.cpp: 
 +      ASSERT(!ec);
What happend to this assert?

WebCore/workers/SharedWorker.cpp:60
 +      KURL scriptURL = worker->resolveURL(url, ec);
Can we get an ec without scriptURL being empty?  Do we need to return like the old code did?

WebCore/workers/Worker.cpp:63
 +      KURL scriptURL = worker->resolveURL(url, ec);
Same ec question.

WebCore/workers/Worker.cpp:67
 +      worker->m_scriptLoader = new WorkerScriptLoader(ResourceRequestBase::TargetIsWorker);
Naked new?

WebCore/workers/Worker.h:57
 +          virtual ~Worker();
!

[Darin Adler]

(In reply to comment #2)
> WebCore/html/FileStreamProxy.cpp:60
>  +      proxy->ref();
> This seems like a bad design.  The way we solved this problem in Chrome is by having specialized Task objects that understood how to ref/deref the objects that they operated upon.  It took a few iterations of the design to get it right, but we got rid of almost all the instances of this (error-prone) pattern.

Good feedback for the engineers working on FileSystemProxy.cpp, Kinuko Yasuda and Jian Li. I believe both work for Google so maybe you can talk to them in person? I wasn’t changing the design; just refactoring to work with the new assertion. I just moved the ref out of the constructor into the create function.

> WebCore/workers/SharedWorker.cpp: 
>  +      ASSERT(!ec);
> What happend to this assert?

I replaced an unimportant assertion with the more important one. The function returns null when it fails. The fact that it also returns an exception code is only for the convenience of JavaScript. I changed the code to assert that the return value is non-null. An alternative would be to keep the !ec assertion and initialize ec to zero. I don’t feel strongly either way.

> WebCore/workers/SharedWorker.cpp:60
>  +      KURL scriptURL = worker->resolveURL(url, ec);
> Can we get an ec without scriptURL being empty?  Do we need to return like the old code did?

We can't. This function always returns a null scriptURL when there is an exception. We don't need code to explicitly check the exception. Generally speaking, exception codes are not a good way for our internal code to work. They are good for one purpose only, to provide exceptions for the binding layer. I think a good general guideline is that when possible, internal code should do nothing with the exceptions except pass them out to the binding layer, and should indicate failure in other ways.

> WebCore/workers/Worker.cpp:63
>  +      KURL scriptURL = worker->resolveURL(url, ec);
> Same ec question.

Same answer.

> WebCore/workers/Worker.cpp:67
>  +      worker->m_scriptLoader = new WorkerScriptLoader(ResourceRequestBase::TargetIsWorker);
> Naked new?

That's an error. This should have an adoptPtr. Once we turn off LOOSE_OWN_PTR it will be an error to omit the adoptPtr. I'll add it, but generally speaking I'll get to those in another patch.

> WebCore/workers/Worker.h:57
>  +          virtual ~Worker();
> !

The destructor was already virtual because the base class, AbstractWorker, has a virtual destructor. I was simply fixing a style mistake. Generally speaking in WebKit we always want to explicitly declare functions virtual rather than letting them be virtual implicitly because they overload virtual function from the base class.

[Kinuko Yasuda]

(In reply to comment #3)
> (In reply to comment #2)
> > WebCore/html/FileStreamProxy.cpp:60
> >  +      proxy->ref();
> > This seems like a bad design.  The way we solved this problem in Chrome is by having specialized Task objects that understood how to ref/deref the objects that they operated upon.  It took a few iterations of the design to get it right, but we got rid of almost all the instances of this (error-prone) pattern.
> 
> Good feedback for the engineers working on FileSystemProxy.cpp, Kinuko Yasuda and Jian Li. I believe both work for Google so maybe you can talk to them in person? I wasn’t changing the design; just refactoring to work with the new assertion. I just moved the ref out of the constructor into the create function.

Thanks for the feedback, that's true that this design is very error-prone.   I'll open a new bug to fix FileStreamProxy design right.

[Darin Adler]

Committed r62696: <http://trac.webkit.org/changeset/62696>

[WebKit Review Bot]

http://trac.webkit.org/changeset/62696 might have broken GTK Linux 64-bit Debug

[Adam Barth]

abarth: http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r62696%20(9359)/results.html
abarth: darin: someone set us up the svg crash
abarth: ASSERTION FAILED: !m_adoptionIsRequired
abarth: (../../JavaScriptCore/wtf/RefCounted.h:37 void WTF::RefCountedBase::ref())

[Darin Adler]

(In reply to comment #7)
> abarth: http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r62696%20(9359)/results.html
> abarth: darin: someone set us up the svg crash
> abarth: ASSERTION FAILED: !m_adoptionIsRequired
> abarth: (../../JavaScriptCore/wtf/RefCounted.h:37 void WTF::RefCountedBase::ref())

I wonder why this is GTK-specific. These same tests don't seem to have this problem on the non-GTK platforms. Could someone show me the backtrace so I can fix this?

[Adam Barth]

Seems to also have caused problems on Leopard Debug

platform/mac/plugins/webScriptObject-exception-deadlock.html
http/tests/workers/shared-worker-redirect.html

http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r62785%20(17026)/results.html