Fix a FileWriter abort race by turning an assert into a soft check.

Fix a FileWriter abort race by turning an assert into a soft check.

See https://bugs.webkit.org/show_bug.cgi?id=81861 for the other side of this.

BUG=119417
TEST=existing tests get less flaky

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=129488

[ericu, 2012-03-22 02:15:58]

David, do you have time to take a look at this [and the webkit side]?  I think
you're the most familiar with the code.

Thanks!

	Eric

[levin, 2012-03-22 03:18:25]

On 2012/03/22 02:15:58, ericu wrote:
> David, do you have time to take a look at this [and the webkit side]?  I think
> you're the most familiar with the code.
> 
> Thanks!
> 
> 	Eric

Would you elaborate a little more on the problem details and how this fixes it?

Thanks!

[ericu, 2012-03-26 23:40:47]

On Wed, Mar 21, 2012 at 8:18 PM,  <levin@chromium.org> wrote:
> Would you elaborate a little more on the problem details and how this fixes
> it?

The WorkerFileWriterCallbacksBridge helps calls from FileWriter ->
WebFileWriterBase jump from the context thread to the main thread, and
helps responses jump the other way.  Because of that asynchronous
link, it's really hard to make sure that an abort/cancel call is
canceling an operation that's still in progress.  The success/failure
response might already be on the way back, and the cancel and success
might pass each other at the bridge while hopping threads.  We could
try to prevent that using cross-thread shared state and a mutex, but
it'd be pretty slow + clunky, and setup and teardown would be tricky.
Instead, I'm making FileWriter more robust to success coming back when
an abort was requested, and this CL makes WebFileWriterBase more
robust to cancel coming in when no job is in progress.

This does mean that we'll just silently drop completely crazy cancel
calls [which weren't caused by that race condition] rather than
asserting.  So we might not catch certain types of bugs here.  But at
least the cancel won't propagate further.

[levin, 2012-03-26 23:45:48]

LGTM

ok, so WebFileWriterBase is used solely on thread 1 and may have changed state
from A to B and sends a message to thread 2.

On thread 2, it may think that things are still in state A so it sends a message
to thread 1.

The out of sync states are now handled on thread 1 by this if.

So LGTM if what I'm saying is correct.

[ericu, 2012-03-27 00:04:08]

On 2012/03/26 23:45:48, levin wrote:
> LGTM
> 
> ok, so WebFileWriterBase is used solely on thread 1 and may have changed state
> from A to B and sends a message to thread 2.
> 
> On thread 2, it may think that things are still in state A so it sends a
message
> to thread 1.
> 
> The out of sync states are now handled on thread 1 by this if.
> 
> So LGTM if what I'm saying is correct.

Yes, that's it exactly.  Thanks!

[commit-bot: I haz the power, 2012-03-27 00:05:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/9764018/1

[commit-bot: I haz the power, 2012-03-27 02:47:28]

Try job failure for 9764018-1 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-28 17:19:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/9764018/1

[commit-bot: I haz the power, 2012-03-28 21:18:04]

Change committed as 129488