Bug 81861 – Two race conditions in FileWriter

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric U.]

FileWriter has two race conditions

[Eric U.]

1) An abort on its way to the backend might, in the Chromium/worker implementation, pass a success or failure message coming the other way.  Then we'd request an abort with no task pending, which the backend won't like.
2) If the user with a write in progress calls abort/write/abort quickly enough, we'll actually send two aborts to the backend, where one is the right number.

The Chromium implementation should just drop any extra calls [I'll do that fix too, but it's not in the WebKit repo], but FileWriter should also be robust to success/non-abort-failure responses when it's sent an abort.

[Eric U.]

Created attachment 133164 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 133164 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133164&action=review

> Source/WebCore/ChangeLog:13
> +        it appropriately.

Could you elaborate a little more about what could (unexpectedly) happen without this change?

> Source/WebCore/Modules/filesystem/FileWriter.cpp:195
> +        completeAbort();

Can we early exit here and remove else in line 196?

> Source/WebCore/Modules/filesystem/FileWriter.cpp:197
> +        ASSERT(m_readyState == WRITING);

Is there a chance this is called after stop()?

> Source/WebCore/Modules/filesystem/FileWriter.cpp:223
> +        completeAbort();

ditto.

> Source/WebCore/Modules/filesystem/FileWriter.cpp:240
> +        completeAbort();

ditto.

> Source/WebCore/Modules/filesystem/FileWriter.cpp:245
>          m_blobBeingWritten.clear();

Do we need to change operationInProgress to None here?

> Source/WebCore/Modules/filesystem/FileWriter.h:120
>      ReadyState m_readyState;

Now we have two (or three) variables to keep status.
We sometimes only check ReadyState while do also check progress/queued operations other times and it makes me feel nervous.
Is it possible to submerge m_readyState into progress/queued operation status?  Looks like we can return WRITING status if progress == Write||Truncate||Abort?
(DONE/INIT needs one more operation status though)

[David Levin]

Comment on attachment 133164 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133164&action=review

I agree with Kinuko so this at least needs a few more comments (in the ChangeLog).

> Source/WebCore/Modules/filesystem/FileWriter.cpp:127
> +    } else {

doOperation(OperationWrite);

> Source/WebCore/Modules/filesystem/FileWriter.cpp:168
> +    } else {

doOperation(OperationTruncate);

>> Source/WebCore/Modules/filesystem/FileWriter.cpp:195
>> +        completeAbort();
> 
> Can we early exit here and remove else in line 196?

And that should make the diff less as well :)

> Source/WebCore/Modules/filesystem/FileWriter.cpp:281
> +    default:

Don't add a default to a switch for an enum so that the compile will catch unhandled enums. 

In this case if you wish to assert, do it after the switch and change the break's to return's so they don't hit the assert (and add the new enum value in here and just break for it).

[Alexey Proskuryakov]

This looks like cross-platform code, why [chromium] is title?

[Adam Barth]

(In reply to comment #5)
> This looks like cross-platform code, why [chromium] is title?

We've been having trouble with folks using the [Chromium] tag too often in the title of bugs.  I've tried to correct it when I've noticed.  If you'd be willing to help out educating folks when to use the tag, that would be beneficial.

[Eric U.]

Created attachment 133923 [details]
Patch

[Eric U.]

Comment on attachment 133164 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133164&action=review

>> Source/WebCore/ChangeLog:13
>> +        it appropriately.
> 
> Could you elaborate a little more about what could (unexpectedly) happen without this change?

Done.

>> Source/WebCore/Modules/filesystem/FileWriter.cpp:127
>> +    } else {
> 
> doOperation(OperationWrite);

Fixed, thanks.

>> Source/WebCore/Modules/filesystem/FileWriter.cpp:168
>> +    } else {
> 
> doOperation(OperationTruncate);

Fixed.

>>> Source/WebCore/Modules/filesystem/FileWriter.cpp:195
>>> +        completeAbort();
>> 
>> Can we early exit here and remove else in line 196?
> 
> And that should make the diff less as well :)

Done.

>> Source/WebCore/Modules/filesystem/FileWriter.cpp:197
>> +        ASSERT(m_readyState == WRITING);
> 
> Is there a chance this is called after stop()?

Yes, thanks for the catch!  I've changed stop to use the same code, so now when the completion comes after stop, it sees the same setup, and everything shuts down nicely.

>> Source/WebCore/Modules/filesystem/FileWriter.cpp:223
>> +        completeAbort();
> 
> ditto.

Done.

>> Source/WebCore/Modules/filesystem/FileWriter.cpp:240
>> +        completeAbort();
> 
> ditto.

Done.

>> Source/WebCore/Modules/filesystem/FileWriter.cpp:245
>>          m_blobBeingWritten.clear();
> 
> Do we need to change operationInProgress to None here?

Yes!  Done.

>> Source/WebCore/Modules/filesystem/FileWriter.cpp:281
>> +    default:
> 
> Don't add a default to a switch for an enum so that the compile will catch unhandled enums. 
> 
> In this case if you wish to assert, do it after the switch and change the break's to return's so they don't hit the assert (and add the new enum value in here and just break for it).

Got it; done.  But I've rolled OperationAbort in to share the code.

>> Source/WebCore/Modules/filesystem/FileWriter.h:120
>>      ReadyState m_readyState;
> 
> Now we have two (or three) variables to keep status.
> We sometimes only check ReadyState while do also check progress/queued operations other times and it makes me feel nervous.
> Is it possible to submerge m_readyState into progress/queued operation status?  Looks like we can return WRITING status if progress == Write||Truncate||Abort?
> (DONE/INIT needs one more operation status though)

I'm not sure whether we can reduce by one variable or not.  I suspect not, given that the spec puts rather strict limits on when readyState changes.  However, the spec is just about to change, and I don't think it's worth the effort of doing that optimization before we see exactly what Arun puts in there.  I suggest we revisit the idea when I roll in the next spec change, as it would certainly be nice to drop a variable.

[Eric U.]

(In reply to comment #5)
> This looks like cross-platform code, why [chromium] is title?

Sorry about that--the reason I did it was that the actual assert/crash is only in the Chromium implementation.  I'm changing cross-platform code in ways that should make it more robust for any implementer, but there's no actual error in non-Chromium code that I know of.

I agree that I miscategorized it.

[David Levin]

Comment on attachment 133923 [details]
Patch

Looks ok to me. I think Kinuko did a better job than I did reviewing this so I'd wait for her ok also if possible.

[Eric U.]

(In reply to comment #10)
> (From update of attachment 133923 [details])
> Looks ok to me. I think Kinuko did a better job than I did reviewing this so I'd wait for her ok also if possible.

Sure, will do.  Thanks.

[Kinuko Yasuda]

Comment on attachment 133923 [details]
Patch

LGTM too.

View in context: https://bugs.webkit.org/attachment.cgi?id=133923&action=review

> Source/WebCore/ChangeLog:31
> +        robust to extra abort calls.

Thanks, now it's much clearer!

[Eric U.]

Committed r112445: <http://trac.webkit.org/changeset/112445>