[chromium-dev] [Mac] Use scoped_ptr and scoped_nsobject
We discovered this morning (almost by accident) that the gcc setting GCC_OBJC_CALL_CXX_CDTORS has been enabled for us since our switch to 4.2 last summer. This means that destructors on C++ objects will be called when the objective-c object containing it goes away (this wasn't the case with past Objective-C runtimes). I am about to check in a CL that makes it explicit across our xcode configs so we don't repeat the argument Mark and I had this morning :-) This means that we are now free to use scoped_ptr, scoped_array, scoped_nsobject, etc in our @interfaces to syntactically identify strong references and prevent memory leaks due to programmer error. I encourage people to use these in their code, and fix them when they run across strong references that would benefit from a simple conversion. I converted BrowserWindowController and verified that indeed both dtors and deallocs are correctly called. It took like 2 minutes. Let me know if you have any questions, and be sure to remind your fellow coders about these classes when doing code reviews. -- Mike Pinkerton Mac Weenie pinker...@google.com --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: [Mac] Use scoped_ptr and scoped_nsobject
There is one caveat here that I found after running into some issues with the unit tests. When you convert members from pointers to scoped pointers, even if you keep the destruction ordering consistent (by re-ordering the members in the @interface decl), the objects are destructed much later than before. eg, before when things being cleaned up from, say BrowserWindowController dealloc (ie, before the call to super dealloc), now the destruction chain looks like (from the bottom up): ... now things owned by scoped_ptrs are destroyed here 5 org.chromium.Chromium 0x00152f4a -[BrowserWindowController .cxx_destruct] + 48 6 libobjc.A.dylib 0x94a709d0 object_cxxDestructFromClass + 111 7 libobjc.A.dylib 0x94a71683 _internal_object_dispose + 34 8 com.apple.Foundation0x95996570 NSDeallocateObject + 224 9 com.apple.AppKit0x93d31ba6 -[NSResponder dealloc] + 130 10 com.apple.AppKit0x93ebad26 -[NSWindowController dealloc] + 449 ... before things were destroyed here... 11 org.chromium.Chromium 0x00153e88 -[BrowserWindowController dealloc] + 62 12 com.apple.AppKit0x93dcdb22 -[NSWindowController release] + 158 Note that the C++ objects are being released long *after* the window controller is already gone. If anything was holding a weak reference to the NSWindow, they're in big trouble unless they've retained it (which was the bug I discovered earlier). So these classes are still wonderful and self-documenting and full of goodness, but you should make double-sure you understand what's going on when you do a conversion. -- Mike Pinkerton Mac Weenie pinker...@google.com --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Microsoft symbol proxy can't retrieve debug symbols
Interesting fix ;) On our side, Evan wrote a little apache mod rewrite script that makes a directory case insensitive. It's now live on build.chromium.org. Do you think you can verify that the problem is really fixed by trying again without your fix? Thanks, Nicolas On Thu, Apr 9, 2009 at 4:48 PM, ptr727 pieter.vilj...@gmail.com wrote: After a bit of digging and coding I have a working workaround for the problem. Read about it here: http://blog.insanegenius.com/2009/04/broken-symbol-proxy.html --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Mac/Linux heads-up: RenderWidgetHostView changes needed
Avi/Adam, you got nominated on IRC as relevant people for this. In http://codereview.chromium.org/66013 I'm changing the way some of the RenderWidgetHost[View] functions work slightly. I need to coordinate to get Mac and Linux changes to accommodate this in (I can't land my patch as-is because of Linux try failures due to a new DCHECK, for example). The gist of the change is that RenderWidgetHostView::DidPaintRect() (which generally triggers a paint) may be called back from RenderWidgetHost::GetBackingStore() (which is generally called from inside a paint). This can cause recursion and/or overpainting. I have tried to add comment on this function's declaration explaining what's going on in more detail and what platforms should be doing when they receive this call. And the patch has changes for Windows so you can see what we did there (call GetBackingStore() before getting our invalid rect, and then in DidPaintRect() just invalidate in this case, so that on return to painting we immediately pick up the new, larger invalid rect and paint the whole thing). It's not clear to me that this approach will work on all OSes, which is why I'm not just making this change myself. Please let me know if you understand this patch enough to make appropriate changes, and how to coordinate landing various bits so I don't break anyone. PK --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Microsoft symbol proxy can't retrieve debug symbols
Hi Nicolas I confirmed your fix works, I can now get Chrome symbols without my workaround. Maybe Mozilla / Firefox can do the same on their server? Does your change make the filename case insensitive as well? I think all your files are lowercase, but in case they do mix case, the same problem will happen. Thanks Pieter On Apr 10, 12:10 pm, Nicolas Sylvain nsylv...@chromium.org wrote: Interesting fix ;) On our side, Evan wrote a little apache mod rewrite script that makes a directory case insensitive. It's now live on build.chromium.org. Do you think you can verify that the problem is really fixed by trying again without your fix? Thanks, Nicolas On Thu, Apr 9, 2009 at 4:48 PM, ptr727 pieter.vilj...@gmail.com wrote: After a bit of digging and coding I have a working workaround for the problem. Read about it here: http://blog.insanegenius.com/2009/04/broken-symbol-proxy.html --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] [linux] GTK and multi-threading
glib and x aren't thread-safe. To work around this our intention is to
only call into them from the UI thread. Unfortunately we currently
don't follow through on this.
To query the clipboard the renderer sends a sync IPC call to the
browser. The browser handles this call on the IO thread (in
resource_message_filter.cc), where it calls into base::Clipboard. On
linux this makes copious use of GTK+ functions (which in turn wrap
calls to X). Oops! We'd like to be able to proxy off the calls to the
UI thread, but I hear handling a sync IPC message on the UI thread is
a nono. A couple options have been suggested, although neither is
really all that appealing:
a) wrap the clipboard calls in gtk_threads_{enter,leave}(). This makes
the calls thread safe, but it does so by locking a process-wide mutex,
which will block our UI thread's main loop.
b) Create a second UI thread with its own X connection. Adam suggested
this one so he's better able than I to explain the implications.
Anyone with thoughts, advice, insight?
p.s. So yes, webview copy-pasta is currently very dicey. You can
always use the selection clipboard though, as long as you only care
about text.
-- Evan Stade
--~--~-~--~~~---~--~~
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
-~--~~~~--~~--~--~---
[chromium-dev] Re: [linux] GTK and multi-threading
On Fri, Apr 10, 2009 at 12:42 PM, Evan Stade est...@chromium.org wrote:
a) wrap the clipboard calls in gtk_threads_{enter,leave}(). This makes
the calls thread safe, but it does so by locking a process-wide mutex,
which will block our UI thread's main loop.
I personally think this is OK. These calls should be on the UI
thread and we'd put them there if we could. So having them on another
thread blocking the UI thread seems equivalent. We just need to be
careful to limit the locking to just that.
Brett
--~--~-~--~~~---~--~~
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
-~--~~~~--~~--~--~---
[chromium-dev] Re: Mac/Linux heads-up: RenderWidgetHostView changes needed
On Fri, Apr 10, 2009 at 12:28 PM, Peter Kasting pkast...@google.com wrote: Please let me know if you understand this patch enough to make appropriate changes, and how to coordinate landing various bits so I don't break anyone. PK I patched this in and started fixing things and ended up writing the reverse of your patch. Myabe you want to #ifdef around your changes? I don't get why you want to do this. We have this situation: X sends an expose message to us We ask for a BackingStore We don't have one, so we start a timed wait on the renderer to send us a paint We get the paint, so we create the backing store and copy the bitmap to it. With your patch: We then call back into GTK to invalidate the rect again! Error! Your comment suggests that you want to do more than invalidate in DidPaintRect because of WM_PAINT starvation. I don't get how this helps you. In the case that you're talking about, we're dealing with a message from the renderer without the expose handling on the stack and then that guard that you removed doesn't even come into play. AGL --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Microsoft symbol proxy can't retrieve debug symbols
On Fri, Apr 10, 2009 at 12:40 PM, ptr727 pieter.vilj...@gmail.com wrote: Hi Nicolas I confirmed your fix works, I can now get Chrome symbols without my workaround. Maybe Mozilla / Firefox can do the same on their server? Does your change make the filename case insensitive as well? I think all your files are lowercase, but in case they do mix case, the same problem will happen. Yes, everything should be case insensitive. I'll try to talk to the mozilla guys. Nicolas Thanks Pieter On Apr 10, 12:10 pm, Nicolas Sylvain nsylv...@chromium.org wrote: Interesting fix ;) On our side, Evan wrote a little apache mod rewrite script that makes a directory case insensitive. It's now live on build.chromium.org. Do you think you can verify that the problem is really fixed by trying again without your fix? Thanks, Nicolas On Thu, Apr 9, 2009 at 4:48 PM, ptr727 pieter.vilj...@gmail.com wrote: After a bit of digging and coding I have a working workaround for the problem. Read about it here: http://blog.insanegenius.com/2009/04/broken-symbol-proxy.html --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: Mac/Linux heads-up: RenderWidgetHostView changes needed
On Fri, Apr 10, 2009 at 1:07 PM, Adam Langley a...@chromium.org wrote: X sends an expose message to us We ask for a BackingStore We don't have one, so we start a timed wait on the renderer to send us a paint We get the paint, so we create the backing store and copy the bitmap to it. With your patch: We then call back into GTK to invalidate the rect again! Error! The question is, has GTK already validated the rect it sent us to paint? If so, we should just enlarge the rect that the paint function will redraw; if not, we should both do that and ensure that that larger rect gets validated. (On Windows, the case is the latter.) In neither case do we really want to come out of painting with a dirty rect from this. Your comment suggests that you want to do more than invalidate in DidPaintRect because of WM_PAINT starvation. I don't get how this helps you. In the case that you're talking about, we're dealing with a message from the renderer without the expose handling on the stack and then that guard that you removed doesn't even come into play. When you paint, the paint function will ask for the updated backing store, which potentially needs to go to the renderer and wait (if it is dirty). When it comes back, with the guards removed, you call DidPaintRect(), which calls Paint(), which can recurse because the backing store may still need updating (e.g. we got an ack of a previous resize but another is waiting, or similar). Darin came up with some thought experiments in which this recursion happens effectively infinitely. On the other hand, if you fix this by making DidPaintRect() not paint, then you'll invalidate the area, but potentially never get a paint callback unless your event system guarantees that you'll paint within a certain period no matter how much the renderer is eating the CPU and spamming you with updates. In short, DidPaintRect() painting rate-limits the renderer from starving paints, and being clever about how we paint and invalidate prevents infinite recursion and/or overpainting during this. I strongly suggest picking Darin's brain directly for more detail as he has a stronger grasp of this than I do :) PK --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: [linux] GTK and multi-threading
On Fri, Apr 10, 2009 at 1:03 PM, Brett Wilson bre...@chromium.org wrote:
On Fri, Apr 10, 2009 at 12:42 PM, Evan Stade est...@chromium.org wrote:
a) wrap the clipboard calls in gtk_threads_{enter,leave}(). This makes
the calls thread safe, but it does so by locking a process-wide mutex,
which will block our UI thread's main loop.
I personally think this is OK. These calls should be on the UI
thread and we'd put them there if we could. So having them on another
thread blocking the UI thread seems equivalent. We just need to be
careful to limit the locking to just that.
If in Linux plugin windows block the parent window (as they do on Windows),
then this will lead to a deadlock just as if the message was processed on
the UI thread.
Brett
--~--~-~--~~~---~--~~
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
-~--~~~~--~~--~--~---
[chromium-dev] Re: Creating bugs from test_expectations.txt
On Thu, Apr 9, 2009 at 6:49 PM, Ojan Vafai o...@google.com wrote: At a quick glance, this looks great. I didn't look over every bug, but the ones I did look at look good. Yep. You'll want some sort of default description for the ones that have none. 200+ bugs is certainly too many, but that's no reason not to file them. (Sorry, couldn't resist. Seriously, yes, definitely file them. Better in the issue tracker than getting lost.) It would be great to check in a version of this script that we could run when a number of tests fail (e.g. when doing a bad webkit merge). That way, we can add them all to the local test_expectations.txt file and have it spit out the new results. Fixing the file we have and moving forward are slightly different use cases, but yes. In the long run, we shouldn't need any script to fix an existing bug-less expectations file, only the part that adds bugs for newly added tests. I'm not sure whether that part should be controlled by adding the tests to the file and having the script file bugs, or making it a fully interactive app: give it a list of files and a description, and it both changes the file and creates a bug. Really, it would be great if the script filed bugs and then just modified test_expectations.txt directly (without committing it). Also, the script should remove any comments it moves into bug descriptions. We should get to a point where all the comments about layout tests are in the bug descriptions themselves instead of in this file. I disagree with this last part. I'd prefer a brief description to remain in the file, with any details in the bug. Certainly that's needed for WONTFIX bugs, where we may not have a bug since there's no work to be tracked, but it's helpful for others too. I've found it frustrating and time-consuming to track down when I see big blocks of failures with no explanations at all. Think of it like the svn checkin comment: enough to have an idea what's going on right there where you need it, with more detail in the bug for when you're really digging. - Pam I think it would be good to get the script checked in first and then run it on the existing test_expectations.txt file. Ojan On Thu, Apr 9, 2009 at 6:42 PM, Glenn Wilson gwil...@google.com wrote: Hi Pam Ojan, I wrote a script that would extract all of the layout tests from test_expectations.txt that we haven't marked as WONTFIX and don't have a bug number. I also tried a simple heuristic to get the context of the layout test via nearby commentsit's not perfect, and I'll have to change some of them by hand, but many of the merge comments are getting picked up. I've also hooked up our library for creating demetrius bugs, so getting bugs made for these should be a matter of running the script with different arguments (I hope). What are your thoughts? Is these as descriptive/accurate as we need? Is 200+ bugs too many? Thanks! Glenn --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: [linux] GTK and multi-threading
Option (b) is better, as far as I can tell. On windows, those
messages have their own HWND anyway.
Adam
On Fri, Apr 10, 2009 at 1:15 PM, Ricardo Vargas rvar...@chromium.org wrote:
I opened bug 7379 a while ago to implement option b on the windows side.
On Fri, Apr 10, 2009 at 1:03 PM, Brett Wilson bre...@chromium.org wrote:
On Fri, Apr 10, 2009 at 12:42 PM, Evan Stade est...@chromium.org wrote:
a) wrap the clipboard calls in gtk_threads_{enter,leave}(). This makes
the calls thread safe, but it does so by locking a process-wide mutex,
which will block our UI thread's main loop.
I personally think this is OK. These calls should be on the UI
thread and we'd put them there if we could. So having them on another
thread blocking the UI thread seems equivalent. We just need to be
careful to limit the locking to just that.
Brett
--~--~-~--~~~---~--~~
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
-~--~~~~--~~--~--~---
[chromium-dev] Re: Creating bugs from test_expectations.txt
(Ack...resending) Ok, I have the CL ready, if anyone with Java readability would be willing to do a review, please let me know. more inline... On Fri, Apr 10, 2009 at 2:07 PM, Pam Greene p...@chromium.org wrote: On Thu, Apr 9, 2009 at 6:49 PM, Ojan Vafai o...@google.com wrote: At a quick glance, this looks great. I didn't look over every bug, but the ones I did look at look good. Yep. You'll want some sort of default description for the ones that have none. I ran the script on a small test file...an example bug is here: http://code.google.com/p/chromium/issues/detail?id=9991 There's at least a small bit of indication of where it came from. I also added the line numbers from test_expectations.txt that generated the bug. Additionally, I also wrote the script to then add the created bug numbers to the file. This ends up re-arranging some of the flags (DEBUG DEFER ... etc. -- DEFER DEBUG ...), but all data is retained. 200+ bugs is certainly too many, but that's no reason not to file them. (Sorry, couldn't resist. Seriously, yes, definitely file them. Better in the issue tracker than getting lost.) There are probably some improvements to be had to better group some of the bugs, but it's probably not an order of magnitude improvement. I just didn't want to create tons of bugs that were not useful. It would be great to check in a version of this script that we could run when a number of tests fail (e.g. when doing a bad webkit merge). That way, we can add them all to the local test_expectations.txt file and have it spit out the new results. Fixing the file we have and moving forward are slightly different use cases, but yes. In the long run, we shouldn't need any script to fix an existing bug-less expectations file, only the part that adds bugs for newly added tests. I'm not sure whether that part should be controlled by adding the tests to the file and having the script file bugs, or making it a fully interactive app: give it a list of files and a description, and it both changes the file and creates a bug. There may be a short-term solution and a long-term solution. The short term solution seems to be for someone (assumedly the merger) to add the failing tests to the file, run the script, then commit the file. In the long term, this could be one step, or perhaps be driven right from a roll DEPS to new version of WebKit script...right? Really, it would be great if the script filed bugs and then just modified test_expectations.txt directly (without committing it). Also, the script should remove any comments it moves into bug descriptions. We should get to a point where all the comments about layout tests are in the bug descriptions themselves instead of in this file. I disagree with this last part. I'd prefer a brief description to remain in the file, with any details in the bug. Certainly that's needed for WONTFIX bugs, where we may not have a bug since there's no work to be tracked, but it's helpful for others too. I've found it frustrating and time-consuming to track down when I see big blocks of failures with no explanations at all. Think of it like the svn checkin comment: enough to have an idea what's going on right there where you need it, with more detail in the bug for when you're really digging. Yep, I modified the script so that it will change test_expectations.txt to have the bug number (you'd just need to commit it afterwards). It's pretty easy to for the script to clip out the comments, but I'm reluctant to, because the script may be over/under zealous in what comments it associates with a layout test. Maybe I can add that behavior as another command-line flag. - Pam I think it would be good to get the script checked in first and then run it on the existing test_expectations.txt file. Ojan On Thu, Apr 9, 2009 at 6:42 PM, Glenn Wilson gwil...@google.com wrote: Hi Pam Ojan, I wrote a script that would extract all of the layout tests from test_expectations.txt that we haven't marked as WONTFIX and don't have a bug number. I also tried a simple heuristic to get the context of the layout test via nearby commentsit's not perfect, and I'll have to change some of them by hand, but many of the merge comments are getting picked up. I've also hooked up our library for creating demetrius bugs, so getting bugs made for these should be a matter of running the script with different arguments (I hope). What are your thoughts? Is these as descriptive/accurate as we need? Is 200+ bugs too many? Thanks! Glenn --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] code.google.com down/slow to respond
The issue tracker at code.google.com/p/chromium may be inaccessible or slow to respond for the next few hours. Engineers are working on the issue now, so just be patient. It's a good chance to ignore bugs for a while :) --Mark --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: code.google.com down/slow to respond
And it's back already, faster than I expected. Pardon the spam. On Fri, Apr 10, 2009 at 18:31, Mark Larson (Google) m...@chromium.orgwrote: The issue tracker at code.google.com/p/chromium may be inaccessible or slow to respond for the next few hours. Engineers are working on the issue now, so just be patient. It's a good chance to ignore bugs for a while :) --Mark --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] unable to get chromium source
I have had this constant trouble with getting chromium source. the tarball continually gets disconnected every fw mb(tried on 2 different isp's) and the svn gets svn: REPORT of '/svn/!svn/vcc/default': 200 OK (http://src.chromium.org) after downloading for a while. Here is a message from someone else experiencing same thing. http://www.mail-archive.com/chromium-disc...@googlegroups.com/msg03913.html --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: unable to get chromium source
I've seen this problem too while doing a big checkout, and i'm investigating the issue, but the reason is not clear to me yet. Are you using gclient to get the code? Nicolas On Tue, Apr 7, 2009 at 5:11 PM, GREENie ofsevens...@gmail.com wrote: I have had this constant trouble with getting chromium source. the tarball continually gets disconnected every fw mb(tried on 2 different isp's) and the svn gets svn: REPORT of '/svn/!svn/vcc/default': 200 OK (http://src.chromium.org) after downloading for a while. Here is a message from someone else experiencing same thing. http://www.mail-archive.com/chromium-disc...@googlegroups.com/msg03913.html --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
