As I wrote in https://bugs.webkit.org/show_bug.cgi?id=60655#c3, I don't think the strict PassOwnPtr fix in https://trac.webkit.org/changeset/86296/trunk/Source/WebKit2/Platform/CoreIPC/HandleMessage.h is correct: --->8--- I'm not sure this patch is right. Does that mean we're supposed to leak the replyEncoder if the arguments fail to decode? If this function is supposed take ownership, It seems more likely we should change the parameter to be a PassOwnPtr. I tried chasing this for a while, but it mushroomed out of control pretty fast. I suspect there's a memory management bug here somewhere. For example, consider PluginControllerProxy::didReceiveSyncPluginControllerProxyMessage. Whether this function eventually calls delete on its third argument appears to depend on the MessageID. ---8<--- I don't understand this code well enough to know whether this is a real problem, but the code looks really fishy.
Anders might know.
I think the title of this bug is wrong. I’ll retitle after exploring the code for a moment.
As of this writing, handleMessageDelayed is only used for the HandleMouseEventID and GetPluginProcessConnectionID messages.
Yes, there is a leak here when arguments fail to decode. A good way to fix that would be to put the adoptPtr should be in generated code and have handleMessageDelayed take a PassOwnPtr, as Adam seems to be suggesting. Otherwise, this works fine although it’s a bit awkward. The difference in encoder lifetime is controlled by Connection::dispatchSyncMessage, which calls leakPtr when the reply mode is ManualReply.
Created attachment 93801 [details] Patch
Committed r86812: <http://trac.webkit.org/changeset/86812>
This caused a Python test to fail on all bots :( It's making the entire tree red: http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Tests%29/builds/29491/steps/webkitpy-test/logs/stdio http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/13008/steps/webkitpy-test/logs/stdio http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/13888/steps/webkitpy-test/logs/stdio http://build.webkit.org/builders/Qt%20Linux%20Release/builds/33066/steps/webkitpy-test/logs/stdio http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28WebKit2%20Tests%29/builds/11798/steps/webkitpy-test/logs/stdio Can we either fix the test or revert the change?
(In reply to comment #7) > This caused a Python test to fail on all bots :( It's making the entire tree red: > http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Tests%29/builds/29491/steps/webkitpy-test/logs/stdio > http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/13008/steps/webkitpy-test/logs/stdio > http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/13888/steps/webkitpy-test/logs/stdio > http://build.webkit.org/builders/Qt%20Linux%20Release/builds/33066/steps/webkitpy-test/logs/stdio > http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28WebKit2%20Tests%29/builds/11798/steps/webkitpy-test/logs/stdio > > Can we either fix the test or revert the change? Should be trivial to fix the python test. We just need to update the expected result. I just don’t know how to run the python test!
I see now: 1) I know how to run them. 2) Philippe Normand fixed this for me. Thank you!
test-webkitpy runs all python tests (in 30 seconds). We're working on removing some of the slow ones so that it will be back to < 5 seconds again soon!
(In reply to comment #10) > test-webkitpy runs all python tests (in 30 seconds). We're working on removing some of the slow ones so that it will be back to < 5 seconds again soon! What confused me was the naming here being so different from other related things in WebKit. When our main test runner is named run-webkit-tests, I would have expected the python test runner to be something like run-webkit-python-tests. Or maybe run-webkitpy-tests. I was also surprised to learn that: 1) The name of our python scripts is webkitpy. Seems kinda cryptic to me. 2) There is a webkitpy directory, but the test-webkitpy script tests things outside that directory. I think making things a bit more logical and consistent would have made it easier for me to find what I was looking for.
(In reply to comment #11) > (In reply to comment #10) > > test-webkitpy runs all python tests (in 30 seconds). We're working on removing some of the slow ones so that it will be back to < 5 seconds again soon! > > What confused me was the naming here being so different from other related things in WebKit. There was a long naming debate long ago. I think Chris Jerdonek (who's since left the project) was the main proponent of test-* for test-webkitpy and test-webkitperl. I'm happy to change them to run-python-tests and run-perl-tests if you'd prefer. > When our main test runner is named run-webkit-tests, I would have expected the python test runner to be something like run-webkit-python-tests. Or maybe run-webkitpy-tests. > > I was also surprised to learn that: > > 1) The name of our python scripts is webkitpy. Seems kinda cryptic to me. :shrug: Again, happy to rename. It was just the name picked when webkit-patch (then bugzilla-tool) was split out into more than one file and we needed a name for the directory containing said files. .py is the python file extension. > 2) There is a webkitpy directory, but the test-webkitpy script tests things outside that directory. A sign of age. It used to only test webkitpy. Then we added more python for QueueStatusServer (queues.webkit.org, which uses python on Google App Engine) and then Adam Roben re-used it to test webkit2's python code. These things probably all combine now to add weight to a rename decision. > I think making things a bit more logical and consistent would have made it easier for me to find what I was looking for. Again, happy to make changes if someone files a bug with concrete proposal. I'm completely agnostic to the naming, but I can no longer see the forest between the trees here.
FWIW I am indifferent to whether we rename the files. Personally I'd also vote for moving at least webkitpy (and possibly webkitperl for consistency) somewhere up higher than under Tools/Scripts, and then hopefully being able to move the other python code underneath the new location. But I don't care enough to do much about it :)
Again, they're there for historical reasons. :) Python and perl both have a default import path of ., so its' easy to import webkitpy directly and webkitperl directly. If we moved them up to Tools, we'd have to do ..webkitpy in python and for perl I'm not even sure how you'd do that. :)