Bug 62955 – RefPtr misused as argument type in a few classes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Darin Adler]

RefPtr misused as argument type in a few classes

[Darin Adler]

Created attachment 97727 [details]
Patch

[Darin Adler]

Comment on attachment 97727 [details]
Patch

Clearing flags on attachment: 97727

Committed r89223: <http://trac.webkit.org/changeset/89223>

[Darin Adler]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This patch appears to have broken builds on almost all ports/platforms.

[Mark Rowe (bdash)]

Rolled out in r89224 due to the mass breakage that was caused.

[Darin Adler]

Oops, sorry. Thanks for rolling it out.

No big rush to fix these, I guess I’ll do it later.

[Ryosuke Niwa]

Here's error:
http://build.webkit.org/builders/SnowLeopard%20Intel%20Debug%20%28Build%29/builds/831/steps/compile-webkit/logs/stdio

InspectorBackendDispatcher.cpp was failing to compile because InspectorBackendDispatcher::CSS_setPropertyText was passing RefPtr to setPropertyText, which now takes a raw pointer.  InspectorBackendDispatcher.cpp is generated by CodeGeneratorInspector.pm and the function called typeTraits seems to be responsible for generating type-specific argument but I couldn't figure out how to fix it.  If anything the code that generated InspectorBackendDispatcher::CSS_setPropertyText is around line 460 in CodeGeneratorInspector.pm.

[Darin Adler]

I didn’t realize there was a code generator for the inspector. It’s not urgent to fix this. I can land all the non-inspector parts of this some time.

[Eric Seidel (no email)]

(In reply to comment #8)
> I didn’t realize there was a code generator for the inspector. It’s not urgent to fix this. I can land all the non-inspector parts of this some time.

The inspector also has its very own cookie parser, and javascript object model... just in case.

[Darin Adler]

Created attachment 98091 [details]
Patch

[Darin Adler]

Removed the inspector part of the patch.

[WebKit Review Bot]

The commit-queue encountered the following flaky tests while processing attachment 98091 [details]:

fast/forms/checkValidity-cancel.html bug 63111 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Review Bot]

Comment on attachment 98091 [details]
Patch

Clearing flags on attachment: 98091

Committed r89402: <http://trac.webkit.org/changeset/89402>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

It made 6 tests crash: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20%28Tests%29/r89403%20%2830612%29/results.html

and rolled out by http://trac.webkit.org/changeset/89420

[Ryosuke Niwa]

Comment on attachment 98091 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98091&action=review

> Source/WebCore/websockets/WorkerThreadableWebSocketChannel.cpp:268
> -void WorkerThreadableWebSocketChannel::Bridge::mainThreadCreateWebSocketChannel(ScriptExecutionContext* context, Bridge* thisPtr, RefPtr<ThreadableWebSocketChannelClientWrapper> clientWrapper, const String& taskMode, const KURL& url, const String& protocol)
> +void WorkerThreadableWebSocketChannel::Bridge::mainThreadCreateWebSocketChannel(ScriptExecutionContext* context, Bridge* thisPtr, PassRefPtr<ThreadableWebSocketChannelClientWrapper> clientWrapper, const String& taskMode, const KURL& url, const String& protocol)

Now I see the problem.  clientWrapper is passed to Peer::create and createCallbackTask.

[Darin Adler]

(In reply to comment #16)
> > +void WorkerThreadableWebSocketChannel::Bridge::mainThreadCreateWebSocketChannel(ScriptExecutionContext* context, Bridge* thisPtr, PassRefPtr<ThreadableWebSocketChannelClientWrapper> clientWrapper, const String& taskMode, const KURL& url, const String& protocol)
> 
> Now I see the problem.  clientWrapper is passed to Peer::create and createCallbackTask.

So that function needs to use the prp idiom.

[Darin Adler]

Anyone know why the commit-bot and the cr-linux EWS bot did not catch that?

[Ryosuke Niwa]

(In reply to comment #18)
> Anyone know why the commit-bot and the cr-linux EWS bot did not catch that?

commit-queue uses Chromium Linux bot but Chromium disables workers' tests in DRT because they require multiple processes.  commit-queue also won't catch any JSC regressions because Chromium uses V8 although I had never seen JSC folks using commit-queue before the switch anyway.

[Darin Adler]

(In reply to comment #19)
> I had never seen JSC folks using commit-queue before the switch anyway.

What? If by "JSC folks" you mean "anyone not working on Chromium" then there have been 100s of JSC folks using commit-queue.

I guess you mean people working directly on the JavaScript engine.

[Ryosuke Niwa]

(In reply to comment #20)
> (In reply to comment #19)
> > I had never seen JSC folks using commit-queue before the switch anyway.
> 
> What? If by "JSC folks" you mean "anyone not working on Chromium" then there have been 100s of JSC folks using commit-queue.
> 
> I guess you mean people working directly on the JavaScript engine.

I meant people working on JavaScriptCore.

[Darin Adler]

Created attachment 98385 [details]
Patch

[Darin Adler]

New patch fixes the prp mistake in WorkerThreadableWebSocketChannel::Bridge::mainThreadCreateWebSocketChannel that Ryosuke spotted for me and is the reason this broke the build.

[WebKit Review Bot]

Comment on attachment 98385 [details]
Patch

Clearing flags on attachment: 98385

Committed r89613: <http://trac.webkit.org/changeset/89613>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.