There are some noisy warnings emanating from the NS_ENSURE_TRUE(rollupWidget, false); line in nsWindow.cpp and in corresponding files for other platforms. If that's not an exceptional condition, we should just be doing `if (!rollupWidget) return false;`.

This appears to have been introduced as part of Bug 819888.

Remove the warning, the previous code was just null checking this so looks like it's not needed.

There is an equivalent problem under OS X. Don't land the patch yet, I'll find it for you (give me a minute or two)...

https://hg.mozilla.org/integration/mozilla-inbound/rev/324f9002ab8a Sure, we can take separate patches per platform.

This isn't ready to land. Although the code should be equivalent, applying this patch causes a segfault down inside mozStorageService.cpp when the browser terminates. I suspect the segfault is an unrelated, pre-existing defect that this change somehow exposes, but I don't have time to chase that down right now.

(In reply to Adam Roach [:abr] from comment #5) > This isn't ready to land. Although the code should be equivalent, applying > this patch causes a segfault down inside mozStorageService.cpp when the > browser terminates. I suspect the segfault is an unrelated, pre-existing That shouldn't be related to your patch. I have seen dozen of assertions and crashes for that with the latest debug tinderbox build on 10.7 earlier today.

(In reply to Henrik Skupin (:whimboo) from comment #6) > (In reply to Adam Roach [:abr] from comment #5) > > This isn't ready to land. Although the code should be equivalent, applying > > this patch causes a segfault down inside mozStorageService.cpp when the > > browser terminates. I suspect the segfault is an unrelated, pre-existing > > That shouldn't be related to your patch. I have seen dozen of assertions and > crashes for that with the latest debug tinderbox build on 10.7 earlier today. Yeah, I agree. But right now, that crash will erroneously bisect to this patch. I want to wait for the builds to be more stable before we try landing this. In any case, I've got some higher priority bugs to chase right now, and this is just an annoyance. I'll circle back around to it early next week.

Comment on attachment 697949 [details] [diff] [review] OS X Patch 1.0 Review of attachment 697949 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/cocoa/nsToolkit.mm @@ +167,5 @@ > nsIRollupListener* rollupListener = nsBaseWidget::GetActiveRollupListener(); > NS_ENSURE_TRUE(rollupListener, event); > nsCOMPtr<nsIWidget> rollupWidget = rollupListener->GetRollupWidget(); > + if (!rollupWidget) > + return event; Looks like this was already a simple check and was explicitly converted to a warning NS_ENSURE. I wonder if that was with a reason.

(In reply to Marco Bonardo [:mak] from comment #8) > Comment on attachment 697949 [details] [diff] [review] > OS X Patch 1.0 > > Review of attachment 697949 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: widget/cocoa/nsToolkit.mm > @@ +167,5 @@ > > nsIRollupListener* rollupListener = nsBaseWidget::GetActiveRollupListener(); > > NS_ENSURE_TRUE(rollupListener, event); > > nsCOMPtr<nsIWidget> rollupWidget = rollupListener->GetRollupWidget(); > > + if (!rollupWidget) > > + return event; > > Looks like this was already a simple check and was explicitly converted to a > warning NS_ENSURE. I wonder if that was with a reason. This was the result of a simple copy paste error from the check above. This form of check is fine.

(In reply to Adam Roach [:abr] from comment #5) > Created attachment 697949 [details] [diff] [review] > OS X Patch 1.0 > > This isn't ready to land. Although the code should be equivalent, applying > this patch causes a segfault down inside mozStorageService.cpp when the > browser terminates. You may do a try landing, but I don't see anything dangerous in your patch honestly and I don't see how it could change the frequency of a segfault.

re-assigning to :abr to handle the OSX part. I think if we get a green try run out of that we can just proceed.

windows fix: https://hg.mozilla.org/releases/mozilla-beta/rev/aad6dcd323c3

We should probably close this out and clone this over for osx work now that the windows fix is tracked for beta.

(In reply to Jim Mathies [:jimm] from comment #14) > We should probably close this out and clone this over for osx work now that > the windows fix is tracked for beta. Sure, sounds good to me. I'd hoped to finish figuring out what's going on with the crash and land this, but I've been tied up on other things recently.

cloned to Bug 831342

Marking this [qa?]. Is there anything QA can help with?