Handle has_more correctly in WebFileSystemImpl

Handle has_more correctly in WebFileSystemImpl

Make WaitableCallbackResults ref-counted and a member of WebFileSystemImpl to keep it alive longer than before.
Add CallbacksUnregisterMode argument to RunCallbacks() to keep callbacks registered when necessary.
Initialize next_callbacks_id_ with 1 instead 0 to make 0 usable as an invalid ID.
Implement waitForAdditionalResult().
UnregisterCallbacks() is also responsible to unregister WaitableCallbackResults.

Corresponding patch: https://codereview.chromium.org/178333009/

BUG=347900
TEST=run_webkit_tests.sh http/tests/filesystem/\*
TEST=run_webkit_tests.sh http/tests/inspector/filesystem/\*

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

[hashimoto, 2014-03-06 12:24:05]

Added #ifdef hack to keep compatibility between blink and chrome. (also to the
blink patch)
This should not break the build as long as this patch is committed before the
blink path.
PTAL.

[kinuko, 2014-03-06 13:12:35]

lgtm

https://codereview.chromium.org/188533002/diff/90001/content/child/fileapi/we...
File content/child/fileapi/webfilesystem_impl.cc (right):

https://codereview.chromium.org/188533002/diff/90001/content/child/fileapi/we...
content/child/fileapi/webfilesystem_impl.cc:44: void SetResultsAndSignal(const
base::Closure& results_closure) {
SetResultsAndSignal -> AddResultsAndSignal ?

[nhiroki, 2014-03-06 16:15:31]

FYI: Although this change is conflicting with mine
(https://codereview.chromium.org/102603014/),
please feel free to commit this because your change seems to resolve my issue,
too. Thanks!

[hashimoto, 2014-03-07 02:15:47]

https://codereview.chromium.org/188533002/diff/90001/content/child/fileapi/we...
File content/child/fileapi/webfilesystem_impl.cc (right):

https://codereview.chromium.org/188533002/diff/90001/content/child/fileapi/we...
content/child/fileapi/webfilesystem_impl.cc:44: void SetResultsAndSignal(const
base::Closure& results_closure) {
On 2014/03/06 13:12:35, kinuko wrote:
> SetResultsAndSignal -> AddResultsAndSignal ?

Done.

[hashimoto, 2014-03-07 02:15:55]

The CQ bit was checked by hashimoto@chromium.org

[hashimoto, 2014-03-07 02:18:03]

On 2014/03/06 16:15:31, nhiroki wrote:
> FYI: Although this change is conflicting with mine
> (https://codereview.chromium.org/102603014/),
> please feel free to commit this because your change seems to resolve my issue,
> too. Thanks!
Thanks, I'm going to commit this.

[commit-bot: I haz the power, 2014-03-07 02:19:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/188533002/110001

[hashimoto, 2014-03-07 05:34:14]

Found a race bug and uploaded a new patch.
PTAL

The race was happening when functions are called in the following order:
(IPC is received)
->DispatchResultsClosure()
->(The event is signaled, but no one is waiting)
->WaitableCallbackResults::Run() runs the closure WITHOUT resetting the event.
-> If WaitAndRun() is called after these events, it gets no results because the
event is signaled but the closure vector is empty.

The new patch prevents this from happening by guaranteeing that the event is
signaled iff. the closure vector is not empty.

[kinuko, 2014-03-07 07:59:44]

lgtm, with one note

It looks changing the if condition for m_fileSystem->waitForAdditionalResult()
to while would have the same effect?
Resetting manually feels a bit error prone, though this change looks right to
me.

[hashimoto, 2014-03-07 08:26:57]

On 2014/03/07 07:59:44, kinuko wrote:
> lgtm, with one note
> 
> It looks changing the if condition for m_fileSystem->waitForAdditionalResult()
> to while would have the same effect?
Yes, if we changed WaitableCallbackResults::WaitAndRun() and
waitForAdditionalResult() to return false when |closures| is empty, it would
have the same effect.
But, I prefer keeping the current semantics where waitForAdditionalResult()
returns false only for unrecoverable errors. (i.e. it's no use to retry waiting
if the method returns false)
If we introduce the while loop and change things wrongly in future, the while
loop may block the thread forever, consuming the CPU, retrying
waitForAdditionalResult() for already unregistered callbacksId again and again.

> Resetting manually feels a bit error prone, though this change looks right to
> me.
I agree manual reset is potentially error prone, but it should cause no problem
as long as WaitableCallbackResults keeps its small manageable size.

WDYT?

[kinuko, 2014-03-07 08:47:59]

On 2014/03/07 08:26:57, hashimoto wrote:
> On 2014/03/07 07:59:44, kinuko wrote:
> > lgtm, with one note
> > 
> > It looks changing the if condition for
m_fileSystem->waitForAdditionalResult()
> > to while would have the same effect?
> Yes, if we changed WaitableCallbackResults::WaitAndRun() and
> waitForAdditionalResult() to return false when |closures| is empty, it would
> have the same effect.
> But, I prefer keeping the current semantics where waitForAdditionalResult()
> returns false only for unrecoverable errors. (i.e. it's no use to retry
waiting
> if the method returns false)
> If we introduce the while loop and change things wrongly in future, the while
> loop may block the thread forever, consuming the CPU, retrying
> waitForAdditionalResult() for already unregistered callbacksId again and
again.
> 
> > Resetting manually feels a bit error prone, though this change looks right
to
> > me.
> I agree manual reset is potentially error prone, but it should cause no
problem
> as long as WaitableCallbackResults keeps its small manageable size.
> 
> WDYT?

As my stamp tells I'm ok with this change (esp. if you have preference to do
so), I understand there's pros/cons in this design choice (while loop could be
robust but could be permissive for busy loops).

[kinuko, 2014-03-07 08:53:52]

(I mean, please go ahead landing, I wanted to make sure you surely want to
manually reset it)


On Fri, Mar 7, 2014 at 5:48 PM, <kinuko@chromium.org> wrote:

> On 2014/03/07 08:26:57, hashimoto wrote:
>
>> On 2014/03/07 07:59:44, kinuko wrote:
>> > lgtm, with one note
>> >
>> > It looks changing the if condition for
>>
> m_fileSystem->waitForAdditionalResult()
>
>> > to while would have the same effect?
>> Yes, if we changed WaitableCallbackResults::WaitAndRun() and
>> waitForAdditionalResult() to return false when |closures| is empty, it
>> would
>> have the same effect.
>> But, I prefer keeping the current semantics where
>> waitForAdditionalResult()
>> returns false only for unrecoverable errors. (i.e. it's no use to retry
>>
> waiting
>
>> if the method returns false)
>> If we introduce the while loop and change things wrongly in future, the
>> while
>> loop may block the thread forever, consuming the CPU, retrying
>> waitForAdditionalResult() for already unregistered callbacksId again and
>>
> again.
>
>  > Resetting manually feels a bit error prone, though this change looks
>> right
>>
> to
>
>> > me.
>> I agree manual reset is potentially error prone, but it should cause no
>>
> problem
>
>> as long as WaitableCallbackResults keeps its small manageable size.
>>
>
>  WDYT?
>>
>
> As my stamp tells I'm ok with this change (esp. if you have preference to
> do
> so), I understand there's pros/cons in this design choice (while loop
> could be
> robust but could be permissive for busy loops).
>
> https://codereview.chromium.org/188533002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hashimoto, 2014-03-07 08:56:38]

On 2014/03/07 08:53:52, kinuko wrote:
> (I mean, please go ahead landing, I wanted to make sure you surely want to
> manually reset it)
got it, thanks.
> 
> 
> On Fri, Mar 7, 2014 at 5:48 PM, <mailto:kinuko@chromium.org> wrote:
> 
> > On 2014/03/07 08:26:57, hashimoto wrote:
> >
> >> On 2014/03/07 07:59:44, kinuko wrote:
> >> > lgtm, with one note
> >> >
> >> > It looks changing the if condition for
> >>
> > m_fileSystem->waitForAdditionalResult()
> >
> >> > to while would have the same effect?
> >> Yes, if we changed WaitableCallbackResults::WaitAndRun() and
> >> waitForAdditionalResult() to return false when |closures| is empty, it
> >> would
> >> have the same effect.
> >> But, I prefer keeping the current semantics where
> >> waitForAdditionalResult()
> >> returns false only for unrecoverable errors. (i.e. it's no use to retry
> >>
> > waiting
> >
> >> if the method returns false)
> >> If we introduce the while loop and change things wrongly in future, the
> >> while
> >> loop may block the thread forever, consuming the CPU, retrying
> >> waitForAdditionalResult() for already unregistered callbacksId again and
> >>
> > again.
> >
> >  > Resetting manually feels a bit error prone, though this change looks
> >> right
> >>
> > to
> >
> >> > me.
> >> I agree manual reset is potentially error prone, but it should cause no
> >>
> > problem
> >
> >> as long as WaitableCallbackResults keeps its small manageable size.
> >>
> >
> >  WDYT?
> >>
> >
> > As my stamp tells I'm ok with this change (esp. if you have preference to
> > do
> > so), I understand there's pros/cons in this design choice (while loop
> > could be
> > robust but could be permissive for busy loops).
> >
> > https://codereview.chromium.org/188533002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[hashimoto, 2014-03-07 08:56:45]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-03-07 08:57:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/188533002/150001

[commit-bot: I haz the power, 2014-03-07 15:54:11]

Change committed as 255625