Reenable BrowserTest.WindowOpenClose

Reenable BrowserTest.WindowOpenClose

With the better popup blocker the test didn't work anymore because it
expected the windows to be actually created (event though not shown).

BUG=none
R=bauerb@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214827

[Elliot Glaysher, 2013-07-30 18:13:49]

+estade

https://codereview.chromium.org/21123007/diff/1/ui/base/gtk/owned_widget_gtk.cc
File ui/base/gtk/owned_widget_gtk.cc (left):

https://codereview.chromium.org/21123007/diff/1/ui/base/gtk/owned_widget_gtk....
ui/base/gtk/owned_widget_gtk.cc:41: DCHECK_EQ(G_OBJECT(widget)->ref_count, 1U);
While I really really want this line gone (and is probably wrong now that Ubuntu
is loading multiple other libraries into our process that hold on to our
widgets), have you discussed this with other people? I don't feel comfortable
dropping this w/o discussion because estade has fought to keep this here.

[jochen (gone - plz use gerrit), 2013-07-30 19:27:36]

It's possible that this actually indicates a bug, however, the DCHECK repos 100%
and it's not depending on some test-only condition.

[Evan Stade, 2013-07-30 22:08:50]

https://codereview.chromium.org/21123007/diff/1/ui/base/gtk/owned_widget_gtk.cc
File ui/base/gtk/owned_widget_gtk.cc (left):

https://codereview.chromium.org/21123007/diff/1/ui/base/gtk/owned_widget_gtk....
ui/base/gtk/owned_widget_gtk.cc:41: DCHECK_EQ(G_OBJECT(widget)->ref_count, 1U);
On 2013/07/30 18:13:49, Elliot Glaysher wrote:
> While I really really want this line gone (and is probably wrong now that
Ubuntu
> is loading multiple other libraries into our process that hold on to our
> widgets), have you discussed this with other people? I don't feel comfortable
> dropping this w/o discussion because estade has fought to keep this here.

This DCHECK is not spurious. It was intentionally put here and is a large part
of why this class even exists. Without this check, the class serves a different
purpose: it goes from "OwnedWidget" to "ScopedWidgetReference". It has served us
well for years (although yes it can be annoying). The fact that the DCHECK is
being hit 100% of the time indicates that someone's code is wrong.

[jochen (gone - plz use gerrit), 2013-07-30 22:32:23]

Here's the backtrace:

[13318:13318:0731/003037:3167172758877:FATAL:owned_widget_gtk.cc(41)] Check
failed: ((((GObject*) g_type_check_instance_cast ((GTypeInstance*) ((widget)),
(((GType) ((20) << (2))))))))->ref_count == 1U (2 vs. 1)
 [0x7f50e61880fe] base::debug::StackTrace::StackTrace()
 [0x7f50e61dc0df] logging::LogMessage::~LogMessage()
 [0x7f50e88e9926] ui::OwnedWidgetGtk::Destroy()
 [0x7f50e88e97a5] ui::OwnedWidgetGtk::~OwnedWidgetGtk()
 [0x00000283c76e] GlobalHistoryMenu::~GlobalHistoryMenu()
 [0x000002614bde] GlobalMenuBar::~GlobalMenuBar()
 [0x000002614b69] GlobalMenuBar::~GlobalMenuBar()
 [0x0000025d84b2] base::DefaultDeleter<>::operator()()
 [0x0000025da1e4] base::internal::scoped_ptr_impl<>::~scoped_ptr_impl()
 [0x0000025da1a5] base::internal::scoped_ptr_impl<>::~scoped_ptr_impl()
 [0x0000025da185] scoped_ptr<>::~scoped_ptr()
 [0x0000025d6b55] scoped_ptr<>::~scoped_ptr()
 [0x0000025cd02d] BrowserWindowGtk::~BrowserWindowGtk()
 [0x0000025ccd79] BrowserWindowGtk::~BrowserWindowGtk()
 [0x0000025d75a1] base::DeletePointer<>()
 [0x0000025d881d] base::internal::RunnableAdapter<>::Run()
 [0x0000025d87d5] base::internal::InvokeHelper<>::MakeItSo()
 [0x0000025d8799] base::internal::Invoker<>::Run()
 [0x7f50e6179c0e] base::Callback<>::Run()
 [0x7f50e61ec06c] base::MessageLoop::RunTask()
 [0x7f50e61ec37b] base::MessageLoop::DeferOrRunPendingTask()
 [0x7f50e61ec5a5] base::MessageLoop::DoWork()
 [0x7f50e6158532] base::MessagePumpGlib::RunWithDispatcher()
 [0x7f50e6158b69] base::MessagePumpGlib::Run()
 [0x7f50e61eba67] base::MessageLoop::RunInternal()
 [0x7f50e61eb915] base::MessageLoop::RunHandler()
 [0x7f50e6238322] base::RunLoop::Run()
 [0x000001cfdad9] content::RunThisRunLoop()
 [0x000000a56c3d] base::internal::RunnableAdapter<>::Run()
 [0x000000a56bf8] base::internal::InvokeHelper<>::MakeItSo()
 [0x000000a56b9e] base::internal::Invoker<>::Run()
 [0x000000542e7e] base::Callback<>::Run()
 [0x000001cf7ed1] content::TestNavigationObserver::WaitForObservation()
 [0x0000023a678d]
ui_test_utils::NavigateToURLWithDispositionBlockUntilNavigationsComplete()
 [0x0000023a69ce] ui_test_utils::NavigateToURLBlockUntilNavigationsComplete()
 [0x000000bc7fab] BrowserTest_WindowOpenClose_Test::RunTestOnMainThread()
 [0x000000bc80c2] BrowserTest_WindowOpenClose_Test::RunTestOnMainThread()
 [0x000002397fc4] InProcessBrowserTest::RunTestOnMainThreadLoop()
 [0x0000031aa7f9] content::BrowserTestBase::ProxyRunTestOnMainThreadLoop()
 [0x0000031ab612] base::internal::RunnableAdapter<>::Run()
 [0x0000031ab589] base::internal::InvokeHelper<>::MakeItSo()
 [0x0000031ab545] base::internal::Invoker<>::Run()
 [0x000000542e7e] base::Callback<>::Run()
 [0x000001acd7ca] ChromeBrowserMainParts::PreMainMessageLoopRunImpl()
 [0x000001acc29d] ChromeBrowserMainParts::PreMainMessageLoopRun()
 [0x7f50dd1fe550] content::BrowserMainLoop::CreateThreads()
 [0x7f50dd20663a] content::BrowserMainRunnerImpl::Initialize()
 [0x7f50dd1fb355] content::BrowserMain()
 [0x0000031aa775] content::BrowserTestBase::SetUp()
 [0x000002397274] InProcessBrowserTest::SetUp()
 [0x00000252e1c3] testing::internal::HandleSehExceptionsInMethodIfSupported<>()
 [0x00000252500e] testing::internal::HandleExceptionsInMethodIfSupported<>()
 [0x00000251c8a3] testing::Test::Run()
 [0x00000251cffb] testing::TestInfo::Run()
 [0x00000251d597] testing::TestCase::Run()
 [0x0000025218c5] testing::internal::UnitTestImpl::RunAllTests()
 [0x00000252b253] testing::internal::HandleSehExceptionsInMethodIfSupported<>()
 [0x0000025264fe] testing::internal::HandleExceptionsInMethodIfSupported<>()
 [0x0000025215b4] testing::UnitTest::Run()
 [0x00000248612f] base::TestSuite::Run()
 [0x000000d61fbc] ChromeTestLauncherDelegate::RunTestSuite()
 [0x000001cf673d] content::LaunchTests()

[13318:13318:0731/003037:3167172758877:FATAL:owned_widget_gtk.cc(41)] Check
failed: ((((GObject*) g_type_check_instance_cast ((GTypeInstance*) ((widget)),
(((GType) ((20) << (2))))))))->ref_count == 1U (2 vs. 1)
 [0x7f50e61880fe] base::debug::StackTrace::StackTrace()
 [0x7f50e61dc0df] logging::LogMessage::~LogMessage()
 [0x7f50e88e9926] ui::OwnedWidgetGtk::Destroy()
 [0x7f50e88e97a5] ui::OwnedWidgetGtk::~OwnedWidgetGtk()
 [0x00000283c76e] GlobalHistoryMenu::~GlobalHistoryMenu()
 [0x000002614bde] GlobalMenuBar::~GlobalMenuBar()
 [0x000002614b69] GlobalMenuBar::~GlobalMenuBar()
 [0x0000025d84b2] base::DefaultDeleter<>::operator()()
 [0x0000025da1e4] base::internal::scoped_ptr_impl<>::~scoped_ptr_impl()
 [0x0000025da1a5] base::internal::scoped_ptr_impl<>::~scoped_ptr_impl()
 [0x0000025da185] scoped_ptr<>::~scoped_ptr()
 [0x0000025d6b55] scoped_ptr<>::~scoped_ptr()
 [0x0000025cd02d] BrowserWindowGtk::~BrowserWindowGtk()
 [0x0000025ccd79] BrowserWindowGtk::~BrowserWindowGtk()
 [0x0000025d75a1] base::DeletePointer<>()
 [0x0000025d881d] base::internal::RunnableAdapter<>::Run()
 [0x0000025d87d5] base::internal::InvokeHelper<>::MakeItSo()
 [0x0000025d8799] base::internal::Invoker<>::Run()
 [0x7f50e6179c0e] base::Callback<>::Run()
 [0x7f50e61ec06c] base::MessageLoop::RunTask()
 [0x7f50e61ec37b] base::MessageLoop::DeferOrRunPendingTask()
 [0x7f50e61ec5a5] base::MessageLoop::DoWork()
 [0x7f50e6158532] base::MessagePumpGlib::RunWithDispatcher()
 [0x7f50e6158b69] base::MessagePumpGlib::Run()
 [0x7f50e61eba67] base::MessageLoop::RunInternal()
 [0x7f50e61eb915] base::MessageLoop::RunHandler()
 [0x7f50e6238322] base::RunLoop::Run()
 [0x000001cfdad9] content::RunThisRunLoop()
 [0x000000a56c3d] base::internal::RunnableAdapter<>::Run()
 [0x000000a56bf8] base::internal::InvokeHelper<>::MakeItSo()
 [0x000000a56b9e] base::internal::Invoker<>::Run()
 [0x000000542e7e] base::Callback<>::Run()
 [0x000001cf7ed1] content::TestNavigationObserver::WaitForObservation()
 [0x0000023a678d]
ui_test_utils::NavigateToURLWithDispositionBlockUntilNavigationsComplete()
 [0x0000023a69ce] ui_test_utils::NavigateToURLBlockUntilNavigationsComplete()
 [0x000000bc7fab] BrowserTest_WindowOpenClose_Test::RunTestOnMainThread()
 [0x000000bc80c2] BrowserTest_WindowOpenClose_Test::RunTestOnMainThread()
 [0x000002397fc4] InProcessBrowserTest::RunTestOnMainThreadLoop()
 [0x0000031aa7f9] content::BrowserTestBase::ProxyRunTestOnMainThreadLoop()
 [0x0000031ab612] base::internal::RunnableAdapter<>::Run()
 [0x0000031ab589] base::internal::InvokeHelper<>::MakeItSo()
 [0x0000031ab545] base::internal::Invoker<>::Run()
 [0x000000542e7e] base::Callback<>::Run()
 [0x000001acd7ca] ChromeBrowserMainParts::PreMainMessageLoopRunImpl()
 [0x000001acc29d] ChromeBrowserMainParts::PreMainMessageLoopRun()
 [0x7f50dd1fe550] content::BrowserMainLoop::CreateThreads()
 [0x7f50dd20663a] content::BrowserMainRunnerImpl::Initialize()
 [0x7f50dd1fb355] content::BrowserMain()
 [0x0000031aa775] content::BrowserTestBase::SetUp()
 [0x000002397274] InProcessBrowserTest::SetUp()
 [0x00000252e1c3] testing::internal::HandleSehExceptionsInMethodIfSupported<>()
 [0x00000252500e] testing::internal::HandleExceptionsInMethodIfSupported<>()
 [0x00000251c8a3] testing::Test::Run()
 [0x00000251cffb] testing::TestInfo::Run()
 [0x00000251d597] testing::TestCase::Run()
 [0x0000025218c5] testing::internal::UnitTestImpl::RunAllTests()
 [0x00000252b253] testing::internal::HandleSehExceptionsInMethodIfSupported<>()
 [0x0000025264fe] testing::internal::HandleExceptionsInMethodIfSupported<>()
 [0x0000025215b4] testing::UnitTest::Run()
 [0x00000248612f] base::TestSuite::Run()
 [0x000000d61fbc] ChromeTestLauncherDelegate::RunTestSuite()
 [0x000001cf673d] content::LaunchTests()

[Elliot Glaysher, 2013-07-30 23:00:54]

On 2013/07/30 22:32:23, jochen wrote:
> [13318:13318:0731/003037:3167172758877:FATAL:owned_widget_gtk.cc(41)] Check
> failed: ((((GObject*) g_type_check_instance_cast ((GTypeInstance*) ((widget)),
> (((GType) ((20) << (2))))))))->ref_count == 1U (2 vs. 1)
>  [0x7f50e61880fe] base::debug::StackTrace::StackTrace()
>  [0x7f50e61dc0df] logging::LogMessage::~LogMessage()
>  [0x7f50e88e9926] ui::OwnedWidgetGtk::Destroy()
>  [0x7f50e88e97a5] ui::OwnedWidgetGtk::~OwnedWidgetGtk()
>  [0x00000283c76e] GlobalHistoryMenu::~GlobalHistoryMenu()
>  [0x000002614bde] GlobalMenuBar::~GlobalMenuBar()

I've truncated the stack to the part that matters.

In general, it is entirely valid for any injected gmodule that we don't control
(but which runs in our process) to grab any gobject and increment its refcount
to hold on to it, if for no other reason than KDE, Canonical and the GNOME
Foundation use it constantly to redefine behaviour in other people's glib based
programs. There is no concept of ownership in GTK, only ref counting.
ui::OwnedWidgetGtk sets up a contract that it cannot reliably uphold, as we've
seen previously with libcanaberra, that one KDE theme that held references to
objects, the gtkparasite debugging suite, the other place that libdbusmenu-gtk
was holding on to our menus, and now this.

The fix here which is probably acceptable to estade@ is to go into
global_menu_bar.{cc,h} and global_history_menu.{cc,h} and remove the usage of
OwnedWidgetGtk and manage refcounts manually. The third party libdbusmenu-gtk
library will grab a references to our objects. That's how it works. If we can't
fix it inside OwnedWidgetGtk, it should probably be fixed by manually
refcounting.

[Evan Stade, 2013-07-30 23:13:49]

On 2013/07/30 23:00:54, Elliot Glaysher wrote:
> On 2013/07/30 22:32:23, jochen wrote:
> > [13318:13318:0731/003037:3167172758877:FATAL:owned_widget_gtk.cc(41)] Check
> > failed: ((((GObject*) g_type_check_instance_cast ((GTypeInstance*)
((widget)),
> > (((GType) ((20) << (2))))))))->ref_count == 1U (2 vs. 1)
> >  [0x7f50e61880fe] base::debug::StackTrace::StackTrace()
> >  [0x7f50e61dc0df] logging::LogMessage::~LogMessage()
> >  [0x7f50e88e9926] ui::OwnedWidgetGtk::Destroy()
> >  [0x7f50e88e97a5] ui::OwnedWidgetGtk::~OwnedWidgetGtk()
> >  [0x00000283c76e] GlobalHistoryMenu::~GlobalHistoryMenu()
> >  [0x000002614bde] GlobalMenuBar::~GlobalMenuBar()
> 
> I've truncated the stack to the part that matters.
> 
> In general, it is entirely valid for any injected gmodule that we don't
control
> (but which runs in our process) to grab any gobject and increment its refcount
> to hold on to it, if for no other reason than KDE, Canonical and the GNOME
> Foundation use it constantly to redefine behaviour in other people's glib
based
> programs. There is no concept of ownership in GTK, only ref counting.
> ui::OwnedWidgetGtk sets up a contract that it cannot reliably uphold, as we've
> seen previously with libcanaberra, that one KDE theme that held references to
> objects, the gtkparasite debugging suite, the other place that libdbusmenu-gtk
> was holding on to our menus, and now this.
> 
> The fix here which is probably acceptable to estade@ is to go into
> global_menu_bar.{cc,h} and global_history_menu.{cc,h} and remove the usage of
> OwnedWidgetGtk and manage refcounts manually. The third party libdbusmenu-gtk
> library will grab a references to our objects. That's how it works. If we
can't
> fix it inside OwnedWidgetGtk, it should probably be fixed by manually
> refcounting.

Or create a scoped ref counting class. But yes, it sounds like a case where we
don't actually 'own' the object, so we shouldn't pretend we do.

In general this check is useful for finding leaks, and it's easier to reason
about ownership than ref holding (which is why ref counting is frowned upon in
chrome). So I think the best course of action is leaving OwnedWidgetGtk alone,
but only using it where it makes sense and where we're able.

[jochen (gone - plz use gerrit), 2013-07-31 07:00:56]

ok, I moved the gtk related changes to a different CL

Bernhard, can you review the test change plz?

[Bernhard Bauer, 2013-07-31 08:04:07]

LGTM (you might want to upload again though, I'm getting a chunk mismatch error
for the side-by-side diff).

[jochen (gone - plz use gerrit), 2013-07-31 21:57:49]

Committed patchset #3 manually as r214827 (presubmit successful).