Bug 57136 – removeAllInDirectory() layout test helper method can call successCallback twice

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

[Adam Klein]

removeAllInDirectory() layout test helper method can call successCallback twice

[Adam Klein]

Created attachment 87002 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 87002 [details]
Patch

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

> LayoutTests/http/tests/filesystem/resources/fs-test-util.js:34
>              }

nit: duplicated diff file entry?

> LayoutTests/http/tests/filesystem/resources/fs-test-util.js:-37
> -            else if (this.entriesCount == 0 && this.successCallback)

Handling empty directory case differently is a good idea, but looking at the code again, I'm afraid we have more bugs.

Isn't there a chance we could call callbacks in a sequence like:

- entriesCallback is called with N entries
  - entryRemovedCallback is called N times
  - at the last entry we dispatch successCallback()
- entriesCallback is called again with another M entries
  - entryRemovedCallback is called M times
  - ... (gush)

It is guaranteed that this doesn't happen in the current readDirectory implementation but in theory it looks possible in other embedders.

So we need to do something like this?


  done = false;

  entryRemovedCallback()
  {
     if (--entriesCount == 0 && successCallback && done) {
        successCallback();
        successCallback = null;
     }
  }

  entriesCallback()
  {
     ...
     if (entries.length == 0) {
        if (entriesCount > 0)
           done = true;
        else if (successCallback)
          successCallback();
      }
  }

[Adam Klein]

Created attachment 87168 [details]
Added done bit

[Adam Klein]

Comment on attachment 87002 [details]
Patch

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

>> LayoutTests/http/tests/filesystem/resources/fs-test-util.js:34
>>              }
> 
> nit: duplicated diff file entry?

Not sure what you mean here; for reasons related to how these tests get run, this code is duplicate in two places now. I can work on fixing that, but I suspect it'll take some changes in the scripts.

>> LayoutTests/http/tests/filesystem/resources/fs-test-util.js:-37
>> -            else if (this.entriesCount == 0 && this.successCallback)
> 
> Handling empty directory case differently is a good idea, but looking at the code again, I'm afraid we have more bugs.
> 
> Isn't there a chance we could call callbacks in a sequence like:
> 
> - entriesCallback is called with N entries
>   - entryRemovedCallback is called N times
>   - at the last entry we dispatch successCallback()
> - entriesCallback is called again with another M entries
>   - entryRemovedCallback is called M times
>   - ... (gush)
> 
> It is guaranteed that this doesn't happen in the current readDirectory implementation but in theory it looks possible in other embedders.
> 
> So we need to do something like this?
> 
> 
>   done = false;
> 
>   entryRemovedCallback()
>   {
>      if (--entriesCount == 0 && successCallback && done) {
>         successCallback();
>         successCallback = null;
>      }
>   }
> 
>   entriesCallback()
>   {
>      ...
>      if (entries.length == 0) {
>         if (entriesCount > 0)
>            done = true;
>         else if (successCallback)
>           successCallback();
>       }
>   }

Did something like this, and got rid of firstEntriesCallback as it's really not needed.  See what you think.

[WebKit Commit Bot]

Comment on attachment 87168 [details]
Added done bit

Clearing flags on attachment: 87168

Committed r82204: <http://trac.webkit.org/changeset/82204>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.