Summary: | RefPtr misused as argument type in a few classes | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||
Component: | WebKit Misc. | Assignee: | Darin Adler <darin> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | eric, ossy, rniwa, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 63129 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Darin Adler
2011-06-19 13:49:04 PDT
Created attachment 97727 [details]
Patch
Comment on attachment 97727 [details] Patch Clearing flags on attachment: 97727 Committed r89223: <http://trac.webkit.org/changeset/89223> All reviewed patches have been landed. Closing bug. This patch appears to have broken builds on almost all ports/platforms. Rolled out in r89224 due to the mass breakage that was caused. Oops, sorry. Thanks for rolling it out. No big rush to fix these, I guess I’ll do it later. 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. 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. (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. Created attachment 98091 [details]
Patch
Removed the inspector part of the patch. The commit-queue encountered the following flaky tests while processing attachment 98091 [details]: fast/forms/checkValidity-cancel.html bug 63111 (author: tkent@chromium.org) The commit-queue is continuing to process your patch. Comment on attachment 98091 [details] Patch Clearing flags on attachment: 98091 Committed r89402: <http://trac.webkit.org/changeset/89402> All reviewed patches have been landed. Closing bug. 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 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. (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. Anyone know why the commit-bot and the cr-linux EWS bot did not catch that? (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. (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. (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. Created attachment 98385 [details]
Patch
New patch fixes the prp mistake in WorkerThreadableWebSocketChannel::Bridge::mainThreadCreateWebSocketChannel that Ryosuke spotted for me and is the reason this broke the build. Comment on attachment 98385 [details] Patch Clearing flags on attachment: 98385 Committed r89613: <http://trac.webkit.org/changeset/89613> All reviewed patches have been landed. Closing bug. |