Re: Re: nestable_tasks_allowed on the Mac issues

The way that window-closing happens on Mac requires a loop to be spun
a few times.  They do things kind of like our DeleteSoon().  This
causes some issues for our testing infrastructure because tests want
to create a very controlled environment.

My plan for the bug was to see if there weren't any way to spin only
the stuff necessary, so that things look hermetic WRT Chrome.  I'd
rather not introduce changes to MessageLoop or MessagePump if I can
help it.

-scott

On Wed, Mar 31, 2010 at 3:58 PM, Jim Roskind <jar <at> chromium.org> wrote:
> [resending from my chromium.org account]
> I'm not sure if I have the full context here... but reading the thread, some
> basic philosophy stuff should be stated.
> My asserted bottom line take-away is:  a) Never use nested loops; b) Always
> have an incredibly compelling (no other way) justification if you use nested
> loops; c) go back and figure out how to avoid them anyway.
> The underlying reason is:
>
> a) Nested loops are predominantly evil.  The fundamental reason is that our
> coding practice in an apartment threaded model tends to assume we don't need
> locks, but our code still has iterators (and elements with global purview)
> held on a stack.  When an *arbitrary* task is run in a nested loop (and
> nested loops don't get to pick and choose), then global changes can cause
> pending data references(on the stack above the nested loop) to become
> outdated.  This class of bug is HELL to understand and debug.  It is the
> moral equivalent of dealing with re-entrancy, when you never planned on
> such.  You will swear that memory is being trashed.... but it is merely that

buildbot failure in Chromium on Chromium Linux Builder (dbg-shlib), revision 43288

Re: Re: nestable_tasks_allowed on the Mac issues

Scott, what do you mean by spinning "only the stuff necessary"?

The CFRunLoop is going to be spun by performClose: in  
NSEventTrackingRunLoopMode. In my tests it only gets spun once, but I  
wouldn't depend on that.

Do you mean wrapping the call to -[window performClose:nil] with a   
ScopedNestableTaskAllower?

Closing a window doesn't actually require the loop to be spun, and  
there really is no need for us to call performClose:.

If instead we called our delegate with windowShouldClose: and then  
assuming that returned YES, called -[window close] ourselves directly  
we could avoid this particular case of runloop spin completely.

Interestingly, right now we actually hide our window before we call  
performClose which means that if the delegate should return NO, our  
window won't be "closed" but will be hidden from view.

Cheers,
Dave

On Mar 31, 2010, at 16:07 , Scott Hess wrote:

> The way that window-closing happens on Mac requires a loop to be spun
> a few times.  They do things kind of like our DeleteSoon().  This
> causes some issues for our testing infrastructure because tests want
> to create a very controlled environment.
>
> My plan for the bug was to see if there weren't any way to spin only
> the stuff necessary, so that things look hermetic WRT Chrome.  I'd
> rather not introduce changes to MessageLoop or MessagePump if I can
> help it.
>
> -scott
>
>
> On Wed, Mar 31, 2010 at 3:58 PM, Jim Roskind <jar <at> chromium.org> wrote:
>> [resending from my chromium.org account]
>> I'm not sure if I have the full context here... but reading the  
>> thread, some
>> basic philosophy stuff should be stated.
>> My asserted bottom line take-away is:  a) Never use nested loops;  
>> b) Always
>> have an incredibly compelling (no other way) justification if you  
>> use nested
>> loops; c) go back and figure out how to avoid them anyway.
>> The underlying reason is:
>>
>> a) Nested loops are predominantly evil.  The fundamental reason is  
>> that our
>> coding practice in an apartment threaded model tends to assume we  
>> don't need
>> locks, but our code still has iterators (and elements with global  
>> purview)
>> held on a stack.  When an *arbitrary* task is run in a nested loop  
>> (and
>> nested loops don't get to pick and choose), then global changes can  
>> cause
>> pending data references(on the stack above the nested loop) to become
>> outdated.  This class of bug is HELL to understand and debug.  It  
>> is the
>> moral equivalent of dealing with re-entrancy, when you never  
>> planned on
>> such.  You will swear that memory is being trashed.... but it is  
>> merely that
>> globally accessible data changes in "impossible" ways.
>> b) It is sometimes workable to allow exactly one, and not more,  
>> users of
>> message loops to be nested.  This is *possible* if the *one* task  
>> that is
>> able to create a nested loop is super-duper-careful (read: written as
>> re-entrant code, with no hanging references to global data *all*  
>> the way
>> up-n-down the stack).    Once this option is opened, everyone (in my
>> experience) wants to be "that one task" that is allowed to spin a  
>> nested
>> loop.  When a second person tries to get this dispensation, terrible
>> problems ensue, as the decision to terminate one message loop is  
>> confused
>> with a decision to terminate another, and message loops get stuck  
>> on the
>> stack when the unwind has the wrong order, and on and on.  For  
>> instance, a
>> nested message loop has no way of ending its loop, and completing the
>> pending task above it, when yet another message loop has <sigh>  
>> been nested
>> beneath it :-/.
>> Please, if at all possible, try to not use a nested message loop.   
>> It will
>> only lead to problems.  For testing, Darin and Sky have suggested a  
>> way to
>> work around some previously nested-message-loop-based-tests.   
>> Please try to
>> see if that can be made workable.
>> Thanks in advance,
>> Jim (message loop helper)
>>
>> On Wed, Mar 31, 2010 at 11:48 AM, Mark Mentovai <mark <at> chromium.org>  
>> wrote:
>>>
>>> (Take note of http://crbug.com/38522 if you haven’t already.)
>>>
>>> The real root of the problem is that we do run a nested loop on the
>>> Mac where other platforms don’t.  BrowserWindowCocoa::Close() is
>>> implemented in terms of -[NSWindow performClose:], which spins a
>>> nested loop.
>>>
>>> I’ve suggested in the past that a way around this was similar to  
>>> what
>>> dmac suggests, to refuse to do Chrome “work” in the message pump if
>>> running nested and the message loop isn’t set up to permit it.   
>>> Darin
>>> didn’t want to expose the “nestable allowed?” property to the pump  
>>> in
>>> this way.  I admit that the circular relationship that it creates
>>> between the message pump and message loop makes things a little  
>>> fuzzy.
>>>
>>> dmac’s suggestion seems to be to leave this logic in the loop, and
>>> just refuse to do work in a nested loop (and make sure that the work
>>> gets done with all pump implementations when falling out of the  
>>> nested
>>> loop) instead of having a CHECK.  Darin may have opinions here, too.
>>>
>>> Mark
>>>
>>> Dave MacLachlan wrote:
>>>> So I see a lot of flaky test errors on the Mac with:
>>>>
>>>> message_loop.cc(324) Check failed: nestable_tasks_allowed
>>>>
>>>> 0   browser_tests StackTrace::StackTrace() + 32
>>>> 1   browser_tests logging::LogMessage::~LogMessage() + 63
>>>> 2   browser_tests MessageLoop::RunTask(Task*) + 173
>>>> 3   browser_tests  
>>>> MessageLoop::ProcessNextDelayedNonNestableTask() + 109
>>>> 4   browser_tests MessageLoop::DoIdleWork() + 17
>>>> 5   browser_tests base::MessagePumpCFRunLoopBase::RunIdleWork() +  
>>>> 74
>>>> 6   browser_tests
>>>> base 
>>>> ::MessagePumpCFRunLoopBase::PreWaitObserver(__CFRunLoopObserver*,
>>>> unsigned long, void*) + 23
>>>> 7   CoreFoundation __CFRunLoopDoObservers + 1186
>>>> 8   CoreFoundation  __CFRunLoopRun + 1154
>>>> 9   CoreFoundation CFRunLoopRunSpecific + 452
>>>> 10  CoreFoundation CFRunLoopRunInMode + 97
>>>> 11  HIToolbox RunCurrentEventLoopInMode + 392
>>>> 12  HIToolbox ReceiveNextEventCommon + 354
>>>> 13  HIToolbox BlockUntilNextEventMatchingListInMode + 81
>>>> 14  AppKit _DPSNextEvent + 847
>>>> 15  AppKit -[NSApplication
>>>> nextEventMatchingMask:untilDate:inMode:dequeue:]
>>>> + 156
>>>> 16  AppKit -[NSButtonCell performClick:] + 983
>>>> 17  browser_tests BrowserWindowCocoa::Close() + 170
>>>> 18  browser_tests Browser::CloseWindow() + 80
>>>> 19  browser_tests
>>>> BrowserActionApiTest_IncognitoBasic_Test::RunTestOnMainThread() +  
>>>> 1284
>>>> 20  browser_tests InProcessBrowserTest::RunTestOnMainThreadLoop()  
>>>> + 463
>>>> 21  browser_tests void DispatchToMethod<InProcessBrowserTest, void
>>>> (InProcessBrowserTest::*)()>(InProcessBrowserTest*, void
>>>> (InProcessBrowserTest::*)(), Tuple0 const&) + 56
>>>> 22  browser_tests RunnableMethod<InProcessBrowserTest, void
>>>> (InProcessBrowserTest::*)(), Tuple0>::Run() + 57
>>>> 23  browser_tests MessageLoop::RunTask(Task*) + 220
>>>> 24  browser_tests
>>>> MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask  
>>>> const&) + 53
>>>> 25  browser_tests MessageLoop::DoWork() + 253
>>>> 26  browser_tests base::MessagePumpCFRunLoopBase::RunWork() + 74
>>>> 27  browser_tests  
>>>> base::MessagePumpCFRunLoopBase::RunWorkSource(void*) +
>>>> 23
>>>>
>>>> I don't think we should be running idle work in a nested run loop  
>>>> should
>>>> we?
>>>> Isn't that the whole point of nestable_tasks_allowed? Now we  
>>>> would only
>>>> be
>>>> seeing these errors on the times when the run loop actually has  
>>>> some
>>>> time to
>>>> go to sleep for some reason, so that may explain the flakiness.  
>>>> Most of
>>>> the
>>>> time when running the tests, the runloops won't have any sleep  
>>>> time I'm
>>>> guessing, so PreWaitObserver won't get called. Should we just be
>>>> blocking
>>>> the call do MessageLoop::DoIdleWork if nestable_tasks_allowed is  
>>>> false?
>>>> I
>>>> did this in my local sandbox and didn't see any hangs in  
>>>> quicktime, or
>>>> animations.
>>>
>>> --
>>> Chromium Developers mailing list: chromium-dev <at> chromium.org
>>> View archives, change email options, or unsubscribe:
>>>    http://groups.google.com/a/chromium.org/group/chromium-dev
>>
>> --
>> Chromium Developers mailing list: chromium-dev <at> chromium.org
>> View archives, change email options, or unsubscribe:
>> http://groups.google.com/a/chromium.org/group/chromium-dev
>>

--

-- 
Chromium Developers mailing list: chromium-dev <at> chromium.org
View archives, change email options, or unsubscribe: 
    http://groups.google.com/a/chromium.org/group/chromium-dev

Re: Re: nestable_tasks_allowed on the Mac issues

I mean figuring out what needs to be done so that we don't need the
situation which causes the problem.  Unfortunately, I'm not going to
have time in this week to spend on it, so I have nothing more specific
to give you.

I can already imagine the IRC threads about how someone is leaving Mac
Chrome because we stopped doing the close-button momentary highlight.

-scott

On Wed, Mar 31, 2010 at 5:05 PM, Dave MacLachlan <dmaclach <at> chromium.org> wrote:
> Scott, what do you mean by spinning "only the stuff necessary"?
>
> The CFRunLoop is going to be spun by performClose: in
> NSEventTrackingRunLoopMode. In my tests it only gets spun once, but I
> wouldn't depend on that.
>
> Do you mean wrapping the call to -[window performClose:nil] with a
>  ScopedNestableTaskAllower?
>
> Closing a window doesn't actually require the loop to be spun, and there
> really is no need for us to call performClose:.
>
> If instead we called our delegate with windowShouldClose: and then assuming
> that returned YES, called -[window close] ourselves directly we could avoid
> this particular case of runloop spin completely.
>
> Interestingly, right now we actually hide our window before we call
> performClose which means that if the delegate should return NO, our window
> won't be "closed" but will be hidden from view.
>
> Cheers,
> Dave
>
> On Mar 31, 2010, at 16:07 , Scott Hess wrote:
>
>> The way that window-closing happens on Mac requires a loop to be spun
>> a few times.  They do things kind of like our DeleteSoon().  This
>> causes some issues for our testing infrastructure because tests want
>> to create a very controlled environment.
>>
>> My plan for the bug was to see if there weren't any way to spin only
>> the stuff necessary, so that things look hermetic WRT Chrome.  I'd
>> rather not introduce changes to MessageLoop or MessagePump if I can
>> help it.
>>
>> -scott
>>
>>
>> On Wed, Mar 31, 2010 at 3:58 PM, Jim Roskind <jar <at> chromium.org> wrote:
>>>
>>> [resending from my chromium.org account]
>>> I'm not sure if I have the full context here... but reading the thread,
>>> some
>>> basic philosophy stuff should be stated.
>>> My asserted bottom line take-away is:  a) Never use nested loops; b)
>>> Always
>>> have an incredibly compelling (no other way) justification if you use
>>> nested
>>> loops; c) go back and figure out how to avoid them anyway.
>>> The underlying reason is:
>>>
>>> a) Nested loops are predominantly evil.  The fundamental reason is that
>>> our
>>> coding practice in an apartment threaded model tends to assume we don't
>>> need
>>> locks, but our code still has iterators (and elements with global
>>> purview)
>>> held on a stack.  When an *arbitrary* task is run in a nested loop (and
>>> nested loops don't get to pick and choose), then global changes can cause
>>> pending data references(on the stack above the nested loop) to become
>>> outdated.  This class of bug is HELL to understand and debug.  It is the
>>> moral equivalent of dealing with re-entrancy, when you never planned on
>>> such.  You will swear that memory is being trashed.... but it is merely
>>> that
>>> globally accessible data changes in "impossible" ways.
>>> b) It is sometimes workable to allow exactly one, and not more, users of
>>> message loops to be nested.  This is *possible* if the *one* task that is
>>> able to create a nested loop is super-duper-careful (read: written as
>>> re-entrant code, with no hanging references to global data *all* the way
>>> up-n-down the stack).    Once this option is opened, everyone (in my
>>> experience) wants to be "that one task" that is allowed to spin a nested
>>> loop.  When a second person tries to get this dispensation, terrible
>>> problems ensue, as the decision to terminate one message loop is confused
>>> with a decision to terminate another, and message loops get stuck on the
>>> stack when the unwind has the wrong order, and on and on.  For instance,
>>> a
>>> nested message loop has no way of ending its loop, and completing the
>>> pending task above it, when yet another message loop has <sigh> been
>>> nested
>>> beneath it :-/.
>>> Please, if at all possible, try to not use a nested message loop.  It
>>> will
>>> only lead to problems.  For testing, Darin and Sky have suggested a way
>>> to
>>> work around some previously nested-message-loop-based-tests.  Please try
>>> to
>>> see if that can be made workable.
>>> Thanks in advance,
>>> Jim (message loop helper)
>>>
>>> On Wed, Mar 31, 2010 at 11:48 AM, Mark Mentovai <mark <at> chromium.org>
>>> wrote:
>>>>
>>>> (Take note of http://crbug.com/38522 if you haven’t already.)
>>>>
>>>> The real root of the problem is that we do run a nested loop on the
>>>> Mac where other platforms don’t.  BrowserWindowCocoa::Close() is
>>>> implemented in terms of -[NSWindow performClose:], which spins a
>>>> nested loop.
>>>>
>>>> I’ve suggested in the past that a way around this was similar to what
>>>> dmac suggests, to refuse to do Chrome “work” in the message pump if
>>>> running nested and the message loop isn’t set up to permit it.  Darin
>>>> didn’t want to expose the “nestable allowed?” property to the pump in
>>>> this way.  I admit that the circular relationship that it creates
>>>> between the message pump and message loop makes things a little fuzzy.
>>>>
>>>> dmac’s suggestion seems to be to leave this logic in the loop, and
>>>> just refuse to do work in a nested loop (and make sure that the work
>>>> gets done with all pump implementations when falling out of the nested
>>>> loop) instead of having a CHECK.  Darin may have opinions here, too.
>>>>
>>>> Mark
>>>>
>>>> Dave MacLachlan wrote:
>>>>>
>>>>> So I see a lot of flaky test errors on the Mac with:
>>>>>
>>>>> message_loop.cc(324) Check failed: nestable_tasks_allowed
>>>>>
>>>>> 0   browser_tests StackTrace::StackTrace() + 32
>>>>> 1   browser_tests logging::LogMessage::~LogMessage() + 63
>>>>> 2   browser_tests MessageLoop::RunTask(Task*) + 173
>>>>> 3   browser_tests MessageLoop::ProcessNextDelayedNonNestableTask() +
>>>>> 109
>>>>> 4   browser_tests MessageLoop::DoIdleWork() + 17
>>>>> 5   browser_tests base::MessagePumpCFRunLoopBase::RunIdleWork() + 74
>>>>> 6   browser_tests
>>>>> base::MessagePumpCFRunLoopBase::PreWaitObserver(__CFRunLoopObserver*,
>>>>> unsigned long, void*) + 23
>>>>> 7   CoreFoundation __CFRunLoopDoObservers + 1186
>>>>> 8   CoreFoundation  __CFRunLoopRun + 1154
>>>>> 9   CoreFoundation CFRunLoopRunSpecific + 452
>>>>> 10  CoreFoundation CFRunLoopRunInMode + 97
>>>>> 11  HIToolbox RunCurrentEventLoopInMode + 392
>>>>> 12  HIToolbox ReceiveNextEventCommon + 354
>>>>> 13  HIToolbox BlockUntilNextEventMatchingListInMode + 81
>>>>> 14  AppKit _DPSNextEvent + 847
>>>>> 15  AppKit -[NSApplication
>>>>> nextEventMatchingMask:untilDate:inMode:dequeue:]
>>>>> + 156
>>>>> 16  AppKit -[NSButtonCell performClick:] + 983
>>>>> 17  browser_tests BrowserWindowCocoa::Close() + 170
>>>>> 18  browser_tests Browser::CloseWindow() + 80
>>>>> 19  browser_tests
>>>>> BrowserActionApiTest_IncognitoBasic_Test::RunTestOnMainThread() + 1284
>>>>> 20  browser_tests InProcessBrowserTest::RunTestOnMainThreadLoop() + 463
>>>>> 21  browser_tests void DispatchToMethod<InProcessBrowserTest, void
>>>>> (InProcessBrowserTest::*)()>(InProcessBrowserTest*, void
>>>>> (InProcessBrowserTest::*)(), Tuple0 const&) + 56
>>>>> 22  browser_tests RunnableMethod<InProcessBrowserTest, void
>>>>> (InProcessBrowserTest::*)(), Tuple0>::Run() + 57
>>>>> 23  browser_tests MessageLoop::RunTask(Task*) + 220
>>>>> 24  browser_tests
>>>>> MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) +
>>>>> 53
>>>>> 25  browser_tests MessageLoop::DoWork() + 253
>>>>> 26  browser_tests base::MessagePumpCFRunLoopBase::RunWork() + 74
>>>>> 27  browser_tests base::MessagePumpCFRunLoopBase::RunWorkSource(void*)
>>>>> +
>>>>> 23
>>>>>
>>>>> I don't think we should be running idle work in a nested run loop
>>>>> should
>>>>> we?
>>>>> Isn't that the whole point of nestable_tasks_allowed? Now we would only
>>>>> be
>>>>> seeing these errors on the times when the run loop actually has some
>>>>> time to
>>>>> go to sleep for some reason, so that may explain the flakiness. Most of
>>>>> the
>>>>> time when running the tests, the runloops won't have any sleep time I'm
>>>>> guessing, so PreWaitObserver won't get called. Should we just be
>>>>> blocking
>>>>> the call do MessageLoop::DoIdleWork if nestable_tasks_allowed is false?
>>>>> I
>>>>> did this in my local sandbox and didn't see any hangs in quicktime, or
>>>>> animations.
>>>>
>>>> --
>>>> Chromium Developers mailing list: chromium-dev <at> chromium.org
>>>> View archives, change email options, or unsubscribe:
>>>>   http://groups.google.com/a/chromium.org/group/chromium-dev
>>>
>>> --
>>> Chromium Developers mailing list: chromium-dev <at> chromium.org
>>> View archives, change email options, or unsubscribe:
>>> http://groups.google.com/a/chromium.org/group/chromium-dev
>>>
>
>

--

-- 
Chromium Developers mailing list: chromium-dev <at> chromium.org
View archives, change email options, or unsubscribe: 
    http://groups.google.com/a/chromium.org/group/chromium-dev

Re: Re: nestable_tasks_allowed on the Mac issues

On Mar 31, 2010, at 17:09 , Scott Hess wrote:

> I mean figuring out what needs to be done so that we don't need the
> situation which causes the problem.  Unfortunately, I'm not going to
> have time in this week to spend on it, so I have nothing more specific
> to give you.
>
> I can already imagine the IRC threads about how someone is leaving Mac
> Chrome because we stopped doing the close-button momentary highlight.

We're already there, and nobody seems to have screamed so far 

Do you see a close-button hilight when you close a window in TextEdit?  
I don't.

Cheers,
Dave

--

-- 
Chromium Developers mailing list: chromium-dev <at> chromium.org
View archives, change email options, or unsubscribe: 
    http://groups.google.com/a/chromium.org/group/chromium-dev

buildbot failure in Chromium on Chromium Linux Builder (dbg-shlib), revision 43304

buildbot failure in Chromium on Linux Builder (Views dbg), revision 43304

buildbot failure in Chromium on Linux Builder (ChromiumOS), revision 43308

Re: Re: nestable_tasks_allowed on the Mac issues

Re: Re: nestable_tasks_allowed on the Mac issues