ShapeDetection: Holding all of mock service to prevent GC

ShapeDetection: Make sure tests are running serially
    
There could be more than one MockFaceDetection association with the same
MockFaceDetectionProvider, the first mock service is random Gcd in same
context, so ensure these tests run serially.
    
BUG=707689
Review-Url: https://codereview.chromium.org/2818563006
Cr-Commit-Position: refs/heads/master@{#475335}
Committed: https://chromium.googlesource.com/chromium/src/+/3324464e62044d7d8cfdac18bac1fcb2835cc6ff

[junwei, 2017-04-14 07:34:07]

Patchset #1 (id:1) has been deleted

[junwei, 2017-04-14 07:37:24]

junwei.fu@intel.com changed reviewers:
+ mcasas@chromium.org, rockot@chromium.org, timloh@chromium.org

[mcasas, 2017-04-14 18:02:31]

The CQ bit was checked by mcasas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 18:02:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mcasas, 2017-04-14 18:06:35]

lgtm if rockot@ is happy with it

[commit-bot: I haz the power, 2017-04-14 20:47:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-14 20:47:22]

Dry run: This issue passed the CQ dry run.

[Timothy Loh, 2017-04-18 01:26:06]

timloh@chromium.org changed reviewers:
- timloh@chromium.org

[Timothy Loh, 2017-04-18 01:26:06]

Thanks for fixing; generally no need to send test fixes to the sheriff who added
the test expectation because they won't have much context (removing myself as
reviewer).

[Ken Rockot(use gerrit already), 2017-04-18 22:05:15]

Not LGTM. This definitely does not look correct to me.

I've already fixed the bug where Mojo handle notifications could fire in between
finalization steps and cause a crash. The issue as I understand it now is that
one or more tests does not complete. This implies that the MockFaceDetection
object (or some other JS object) is not living as long as it needs to.

The Binding's sole purpose here is to dispatch incoming calls to the
MockFaceDetection object, so it's wrong to assume a MockFaceDetection could or
should be outlived by its own Binding. It is therefore completely reasonable and
correct for a MockFaceDetection to own its own Binding, and that should not be
changed.

I don't know the tests well enough to understand exactly what's wrong, but
arbitrarily keeping all of these Binding objects alive longer seems like it only
works by luck.

[junwei, 2017-04-19 11:55:26]

Description was changed from

==========
ShapeDetection: Hold binding to fix random crash in layout tests

This random crash also can be reproduced without CL-2771703002 [1] and
CL-2765723002 [2] with following command.
third_party/WebKit/Tools/Scripts/run-webkit-tests -t gn shapedetection/*.html
--verbose  --repeat-each=20 --order=random

The root cause of checking Object isn't a Function is the Object has been GCd,
so the binding of it should be held on.

[1] https://codereview.chromium.org/2771703002
[2] https://codereview.chromium.org/2765723002

BUG=707689
==========

to

==========
ShapeDetection: Enable shapedetection/detection-HTMLCanvasElement.html

This random crash "Check failed: gin::ConvertFromV8(isolate, hidden_value,
&callback)" in waiting_callback.cc has resovled with [1], so enable the skipped
test case.

[1] https://chromium-review.googlesource.com/c/475077/

BUG=707689
==========

[junwei, 2017-04-19 11:55:51]

Description was changed from

==========
ShapeDetection: Enable shapedetection/detection-HTMLCanvasElement.html

This random crash "Check failed: gin::ConvertFromV8(isolate, hidden_value,
&callback)" in waiting_callback.cc has resovled with [1], so enable the skipped
test case.

[1] https://chromium-review.googlesource.com/c/475077/

BUG=707689
==========

to

==========
ShapeDetection: Enable shapedetection/detection-HTMLCanvasElement.html

This random crash "Check failed: gin::ConvertFromV8(isolate, hidden_value,
&callback)"
in waiting_callback.cc has resovled with [1], so enable the skipped test case.

[1] https://chromium-review.googlesource.com/c/475077/

BUG=707689
==========

[junwei, 2017-04-19 12:00:53]

On 2017/04/18 22:05:15, Ken Rockot wrote:
> Not LGTM. This definitely does not look correct to me.
> 
> I've already fixed the bug where Mojo handle notifications could fire in
between
> finalization steps and cause a crash. The issue as I understand it now is that
> one or more tests does not complete. This implies that the MockFaceDetection
> object (or some other JS object) is not living as long as it needs to.
> 
> The Binding's sole purpose here is to dispatch incoming calls to the
> MockFaceDetection object, so it's wrong to assume a MockFaceDetection could or
> should be outlived by its own Binding. It is therefore completely reasonable
and
> correct for a MockFaceDetection to own its own Binding, and that should not be
> changed.
> 
> I don't know the tests well enough to understand exactly what's wrong, but
> arbitrarily keeping all of these Binding objects alive longer seems like it
only
> works by luck.

I totally agree with you. It's time to test the skipped case that has passed in
the
latest code.

[junwei, 2017-04-20 13:26:14]

Patchset #2 (id:40001) has been deleted

[junwei, 2017-04-24 01:13:20]

Patchset #1 (id:20001) has been deleted

[junwei, 2017-05-03 08:27:35]

Description was changed from

==========
ShapeDetection: Enable shapedetection/detection-HTMLCanvasElement.html

This random crash "Check failed: gin::ConvertFromV8(isolate, hidden_value,
&callback)"
in waiting_callback.cc has resovled with [1], so enable the skipped test case.

[1] https://chromium-review.googlesource.com/c/475077/

BUG=707689
==========

to

==========
ShapeDetection: Holding the MockFaceDetection to prevent Binding Object to be
GCd
    
    There could be more than one MockFaceDetection association with the same
    MockFaceDetectionProvider, so Holding the MockFaceDetection to prevent
    Binding Object to be GCd when calling createFaceDetection function
    frequently.
    
    BUG=707689
==========

[junwei, 2017-05-03 08:28:47]

Description was changed from

==========
ShapeDetection: Holding the MockFaceDetection to prevent Binding Object to be
GCd
    
    There could be more than one MockFaceDetection association with the same
    MockFaceDetectionProvider, so Holding the MockFaceDetection to prevent
    Binding Object to be GCd when calling createFaceDetection function
    frequently.
    
    BUG=707689
==========

to

==========
ShapeDetection: Holding the MockFaceDetection to prevent Binding Object to be
GCd
    
There could be more than one MockFaceDetection association with the same
MockFaceDetectionProvider, so Holding the MockFaceDetection to prevent
Binding Object to be GCd when calling createFaceDetection function frequently.
    
BUG=707689
==========

[junwei, 2017-05-03 08:31:46]

Refined the CL, rockot@ mcasas@ PTAL.

[Ken Rockot(use gerrit already), 2017-05-03 16:48:54]

https://codereview.chromium.org/2818563006/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js
(right):

https://codereview.chromium.org/2818563006/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js:21:
this.serviceArray_ = [];
nit: how about "mockServiceInstances_" instead of "serviceArray_"?

https://codereview.chromium.org/2818563006/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js:26:
this.maxDetectedFaces_ = options.max_detected_faces;
Doesn't this effectively cause the same problem as we have with the mock service
instances now? i.e. multiple tests may be using this provider object at the same
time and can thus trample the properties set here?

https://codereview.chromium.org/2818563006/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js:29:
let mock_service = new MockFaceDetection(request, options, this);
nit: Please either just delete this unnecessary line (inline the new at the
push() call) or fix the naming style to mockService

https://codereview.chromium.org/2818563006/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js:34:
return this.buffer_data_;
nit: While you're here, please fix the naming style to bufferData_.

[junwei, 2017-05-04 02:44:20]

On 2017/05/03 16:48:54, Ken Rockot wrote:
>
https://codereview.chromium.org/2818563006/diff/80001/third_party/WebKit/Layo...
> File
> third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js
> (right):
> 
>
https://codereview.chromium.org/2818563006/diff/80001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js:21:
> this.serviceArray_ = [];
> nit: how about "mockServiceInstances_" instead of "serviceArray_"?
Done.

>
https://codereview.chromium.org/2818563006/diff/80001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js:26:
> this.maxDetectedFaces_ = options.max_detected_faces;
> Doesn't this effectively cause the same problem as we have with the mock
service
> instances now? i.e. multiple tests may be using this provider object at the
same
> time and can thus trample the properties set here?
Done.

>
https://codereview.chromium.org/2818563006/diff/80001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js:29:
> let mock_service = new MockFaceDetection(request, options, this);
> nit: Please either just delete this unnecessary line (inline the new at the
> push() call) or fix the naming style to mockService
Done.

>
https://codereview.chromium.org/2818563006/diff/80001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js:34:
> return this.buffer_data_;
> nit: While you're here, please fix the naming style to bufferData_.
Keep original style.

[junwei, 2017-05-04 02:47:48]

Patchset #3 (id:100001) has been deleted

[junwei, 2017-05-04 02:53:35]

Patchset #3 (id:120001) has been deleted

[junwei, 2017-05-04 02:54:39]

>
https://codereview.chromium.org/2818563006/diff/80001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js:34:
> return this.buffer_data_;
> nit: While you're here, please fix the naming style to bufferData_.
Done. Thanks.

[Ken Rockot(use gerrit already), 2017-05-04 08:22:42]

https://codereview.chromium.org/2818563006/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js
(right):

https://codereview.chromium.org/2818563006/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js:28:
getFrameData(index) {
It seems index is always 0, so this is just a more complicated way of doing what
was already done before (i.e. having bufferData_ etc as fields directly on the
provider.)

To be clear, I wasn't sure in my previous comments whether there was actually a
problem with that situation. I was really just asking if that was safe.

Can you please confirm whether or not multiple tests may run concurrently in a
single layout test html i.e. in the same script context?

If the answer is no, then this is safe and what you had before is cleaner. But
then I don't understand what the point of accumulating mockServiceInstances_
would be, because there should be no need for a service instance to remain alive
beyond the duration of a single test.

If the answer is yes, then accumulating mockServiceInstances_ makes sense, but
the code to track buffer data, max detected faces, etc, per test is still wrong.
And this would surprise me because it seems like it would be quite difficult to
do what you want in this sort of environment.

[junwei, 2017-05-07 13:46:44]

On 2017/05/04 08:22:42, Ken Rockot wrote:
>
https://codereview.chromium.org/2818563006/diff/140001/third_party/WebKit/Lay...
> File
> third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js
> (right):
> 
>
https://codereview.chromium.org/2818563006/diff/140001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js:28:
> getFrameData(index) {
> It seems index is always 0, so this is just a more complicated way of doing
what
> was already done before (i.e. having bufferData_ etc as fields directly on the
> provider.)
It's always 0 in current cases, but getMaxDetectedFaces and getFastMode has
different index in detection-options cases.

> To be clear, I wasn't sure in my previous comments whether there was actually
a
> problem with that situation. I was really just asking if that was safe.
> 
> Can you please confirm whether or not multiple tests may run concurrently in a
> single layout test html i.e. in the same script context?
The createFaceDetection is executed in the same script context. I guess the
this.mock_service_ will be re-assigned when second case calling
createFaceDetection, it will cause the first MockFaceDetection to be GCd.

> If the answer is no, then this is safe and what you had before is cleaner. But
> then I don't understand what the point of accumulating mockServiceInstances_
> would be, because there should be no need for a service instance to remain
alive
> beyond the duration of a single test.
> 
> If the answer is yes, then accumulating mockServiceInstances_ makes sense, but
> the code to track buffer data, max detected faces, etc, per test is still
wrong.
> And this would surprise me because it seems like it would be quite difficult
to
> do what you want in this sort of environment.

[Ken Rockot(use gerrit already), 2017-05-07 15:14:48]

On May 7, 2017 6:46 AM, <junwei.fu@intel.com> wrote:

On 2017/05/04 08:22:42, Ken Rockot wrote:
>
https://codereview.chromium.org/2818563006/diff/140001/third_party/WebKit/
LayoutTests/shapedetection/resources/mock-facedetection.js
> File
> third_party/WebKit/LayoutTests/shapedetection/
resources/mock-facedetection.js
> (right):
>
>
https://codereview.chromium.org/2818563006/diff/140001/third_party/WebKit/
LayoutTests/shapedetection/resources/mock-facedetection.js#newcode28
>
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.
js:28:
> getFrameData(index) {
> It seems index is always 0, so this is just a more complicated way of
doing
what
> was already done before (i.e. having bufferData_ etc as fields directly
on the
> provider.)
It's always 0 in current cases, but getMaxDetectedFaces and getFastMode has
different index in detection-options cases.


> To be clear, I wasn't sure in my previous comments whether there was
actually
a
> problem with that situation. I was really just asking if that was safe.
>
> Can you please confirm whether or not multiple tests may run concurrently
in a
> single layout test html i.e. in the same script context?
The createFaceDetection is executed in the same script context. I guess the
this.mock_service_ will be re-assigned when second case calling
createFaceDetection, it will cause the first MockFaceDetection to be GCd.


Right, same script context, but what about concurrency? Does the test
framework not wait for one test to finish before starting the next one?



> If the answer is no, then this is safe and what you had before is
cleaner. But
> then I don't understand what the point of accumulating
mockServiceInstances_
> would be, because there should be no need for a service instance to remain
alive
> beyond the duration of a single test.
>
> If the answer is yes, then accumulating mockServiceInstances_ makes
sense, but
> the code to track buffer data, max detected faces, etc, per test is still
wrong.
> And this would surprise me because it seems like it would be quite
difficult
to
> do what you want in this sort of environment.


https://codereview.chromium.org/2818563006/

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2017-05-07 15:14:49]

On May 7, 2017 6:46 AM, <junwei.fu@intel.com> wrote:

On 2017/05/04 08:22:42, Ken Rockot wrote:
>
https://codereview.chromium.org/2818563006/diff/140001/third_party/WebKit/
LayoutTests/shapedetection/resources/mock-facedetection.js
> File
> third_party/WebKit/LayoutTests/shapedetection/
resources/mock-facedetection.js
> (right):
>
>
https://codereview.chromium.org/2818563006/diff/140001/third_party/WebKit/
LayoutTests/shapedetection/resources/mock-facedetection.js#newcode28
>
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.
js:28:
> getFrameData(index) {
> It seems index is always 0, so this is just a more complicated way of
doing
what
> was already done before (i.e. having bufferData_ etc as fields directly
on the
> provider.)
It's always 0 in current cases, but getMaxDetectedFaces and getFastMode has
different index in detection-options cases.


> To be clear, I wasn't sure in my previous comments whether there was
actually
a
> problem with that situation. I was really just asking if that was safe.
>
> Can you please confirm whether or not multiple tests may run concurrently
in a
> single layout test html i.e. in the same script context?
The createFaceDetection is executed in the same script context. I guess the
this.mock_service_ will be re-assigned when second case calling
createFaceDetection, it will cause the first MockFaceDetection to be GCd.


Right, same script context, but what about concurrency? Does the test
framework not wait for one test to finish before starting the next one?



> If the answer is no, then this is safe and what you had before is
cleaner. But
> then I don't understand what the point of accumulating
mockServiceInstances_
> would be, because there should be no need for a service instance to remain
alive
> beyond the duration of a single test.
>
> If the answer is yes, then accumulating mockServiceInstances_ makes
sense, but
> the code to track buffer data, max detected faces, etc, per test is still
wrong.
> And this would surprise me because it seems like it would be quite
difficult
to
> do what you want in this sort of environment.


https://codereview.chromium.org/2818563006/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[junwei, 2017-05-08 03:13:37]

On 2017/05/07 15:14:49, Ken Rockot wrote:
> On May 7, 2017 6:46 AM, <mailto:junwei.fu@intel.com> wrote:
> 
> On 2017/05/04 08:22:42, Ken Rockot wrote:
> >
> https://codereview.chromium.org/2818563006/diff/140001/third_party/WebKit/
> LayoutTests/shapedetection/resources/mock-facedetection.js
> > File
> > third_party/WebKit/LayoutTests/shapedetection/
> resources/mock-facedetection.js
> > (right):
> >
> >
> https://codereview.chromium.org/2818563006/diff/140001/third_party/WebKit/
> LayoutTests/shapedetection/resources/mock-facedetection.js#newcode28
> >
> third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.
> js:28:
> > getFrameData(index) {
> > It seems index is always 0, so this is just a more complicated way of
> doing
> what
> > was already done before (i.e. having bufferData_ etc as fields directly
> on the
> > provider.)
> It's always 0 in current cases, but getMaxDetectedFaces and getFastMode has
> different index in detection-options cases.
> 
> 
> > To be clear, I wasn't sure in my previous comments whether there was
> actually
> a
> > problem with that situation. I was really just asking if that was safe.
> >
> > Can you please confirm whether or not multiple tests may run concurrently
> in a
> > single layout test html i.e. in the same script context?
> The createFaceDetection is executed in the same script context. I guess the
> this.mock_service_ will be re-assigned when second case calling
> createFaceDetection, it will cause the first MockFaceDetection to be GCd.
> 
> 
> Right, same script context, but what about concurrency? Does the test
> framework not wait for one test to finish before starting the next one?
> 
> 
> 
> > If the answer is no, then this is safe and what you had before is
> cleaner. But
> > then I don't understand what the point of accumulating
> mockServiceInstances_
> > would be, because there should be no need for a service instance to remain
> alive
> > beyond the duration of a single test.
> >
> > If the answer is yes, then accumulating mockServiceInstances_ makes
> sense, but
> > the code to track buffer data, max detected faces, etc, per test is still
> wrong.
> > And this would surprise me because it seems like it would be quite
> difficult
> to
> > do what you want in this sort of environment.
> 
> 
> https://codereview.chromium.org/2818563006/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Yes, the tests in detection-HTMLCanvasElement.html are concurrent.
i add logs in createFaceDetection and detect function, the logs will be output
below

CONSOLE MESSAGE: line 26: ======in createFaceDetection function
CONSOLE MESSAGE: line 26: ======in createFaceDetection function
CONSOLE MESSAGE: line 51: ======in MockFaceDetection detect function
CONSOLE MESSAGE: line 51: ======in MockFaceDetection detect function

[junwei, 2017-05-12 01:01:06]

Refined the CL, rockot@ PTAL thanks.

[junwei, 2017-05-12 01:18:13]

Patchset #4 (id:160001) has been deleted

[junwei, 2017-05-17 05:52:30]

rockot@ PTAL.

[Ken Rockot(use gerrit already), 2017-05-17 15:29:24]

https://codereview.chromium.org/2818563006/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js
(right):

https://codereview.chromium.org/2818563006/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js:26:
this.index = this.mockServiceInstances_.length -1;
You established in your previous reply that multiple tests may run concurrently
in the same context. That would make this code incorrect. i.e. it's possible for
the following sequence of events to occur:

 test1: createFaceDetection()  // index gets set to 0
 test2: createFaceDetection()  // index gets set to 1
 test1: call getFrameData()    // returns instance[1].bufferData_ instead of
instance[0].bufferData_

[junwei, 2017-05-18 00:55:33]

On 2017/05/17 15:29:24, Ken Rockot wrote:
>
https://codereview.chromium.org/2818563006/diff/180001/third_party/WebKit/Lay...
> File
> third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js
> (right):
> 
>
https://codereview.chromium.org/2818563006/diff/180001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js:26:
> this.index = this.mockServiceInstances_.length -1;
> You established in your previous reply that multiple tests may run
concurrently
> in the same context. That would make this code incorrect. i.e. it's possible
for
> the following sequence of events to occur:
> 
>  test1: createFaceDetection()  // index gets set to 0
>  test2: createFaceDetection()  // index gets set to 1
>  test1: call getFrameData()    // returns instance[1].bufferData_ instead of
> instance[0].bufferData_

The behavior is same as maintaining one MockFaceDetection instance that is get
frame data from the latest. It's correct because the bufferData_ is the same
value in these two test cases. My CL is only maintain additionally first Mock
service. Patch Set 3 can be used for different value of BufferData_ with index
arguments, but it's always 0 in current test cases.

[Ken Rockot(use gerrit already), 2017-05-18 01:43:17]

On 2017/05/18 at 00:55:33, junwei.fu wrote:
> On 2017/05/17 15:29:24, Ken Rockot wrote:
> >
https://codereview.chromium.org/2818563006/diff/180001/third_party/WebKit/Lay...
> > File
> >
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js
> > (right):
> > 
> >
https://codereview.chromium.org/2818563006/diff/180001/third_party/WebKit/Lay...
> >
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js:26:
> > this.index = this.mockServiceInstances_.length -1;
> > You established in your previous reply that multiple tests may run
concurrently
> > in the same context. That would make this code incorrect. i.e. it's possible
for
> > the following sequence of events to occur:
> > 
> >  test1: createFaceDetection()  // index gets set to 0
> >  test2: createFaceDetection()  // index gets set to 1
> >  test1: call getFrameData()    // returns instance[1].bufferData_ instead of
> > instance[0].bufferData_
> 
> The behavior is same as maintaining one MockFaceDetection instance that is get
frame data from the latest. It's correct because the bufferData_ is the same
value in these two test cases. My CL is only maintain additionally first Mock
service. Patch Set 3 can be used for different value of BufferData_ with index
arguments, but it's always 0 in current test cases.

Right, so with the tests you have defined today it works (i.e. tests will
consistently pass), but that does not mean the code is correct or that the right
thing is being tested.

There's no point to even having separate storage for bufferData_ etc across test
runs if all tests will use the same index at any given time. It only complicates
the test logic, and it probably guarantees that someone will misinterpret the
behavior and introduce incorrect and flaky tests in the future. 

I think the right thing to do instead may be to ensure these tests run serially
by moving them to separate html files. There is no reliable way for you to link
an incoming interface request (or any subsequent calls on a bound interface) to
a specific test if multiple tests create a FaceDetector connecting to the same
mock service instance.

The only other alternative I can see is if maybe there's a way to express that
async tests in the same html should be run serially.

[junwei, 2017-05-25 07:16:56]

On 2017/05/18 01:43:17, Ken Rockot(use gerrit already) wrote:
> On 2017/05/18 at 00:55:33, junwei.fu wrote:
> > On 2017/05/17 15:29:24, Ken Rockot wrote:
> > >
>
https://codereview.chromium.org/2818563006/diff/180001/third_party/WebKit/Lay...
> > > File
> > >
> third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js
> > > (right):
> > > 
> > >
>
https://codereview.chromium.org/2818563006/diff/180001/third_party/WebKit/Lay...
> > >
>
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js:26:
> > > this.index = this.mockServiceInstances_.length -1;
> > > You established in your previous reply that multiple tests may run
> concurrently
> > > in the same context. That would make this code incorrect. i.e. it's
possible
> for
> > > the following sequence of events to occur:
> > > 
> > >  test1: createFaceDetection()  // index gets set to 0
> > >  test2: createFaceDetection()  // index gets set to 1
> > >  test1: call getFrameData()    // returns instance[1].bufferData_ instead
of
> > > instance[0].bufferData_
> > 
> > The behavior is same as maintaining one MockFaceDetection instance that is
get
> frame data from the latest. It's correct because the bufferData_ is the same
> value in these two test cases. My CL is only maintain additionally first Mock
> service. Patch Set 3 can be used for different value of BufferData_ with index
> arguments, but it's always 0 in current test cases.
> 
> Right, so with the tests you have defined today it works (i.e. tests will
> consistently pass), but that does not mean the code is correct or that the
right
> thing is being tested.
> 
> There's no point to even having separate storage for bufferData_ etc across
test
> runs if all tests will use the same index at any given time. It only
complicates
> the test logic, and it probably guarantees that someone will misinterpret the
> behavior and introduce incorrect and flaky tests in the future. 
> 
> I think the right thing to do instead may be to ensure these tests run
serially
> by moving them to separate html files. There is no reliable way for you to
link
> an incoming interface request (or any subsequent calls on a bound interface)
to
> a specific test if multiple tests create a FaceDetector connecting to the same
> mock service instance.
> 
> The only other alternative I can see is if maybe there's a way to express that
> async tests in the same html should be run serially.

rockot@ PTAL again about making sure these tests run serially, thanks.

[junwei, 2017-05-25 07:24:08]

Description was changed from

==========
ShapeDetection: Holding the MockFaceDetection to prevent Binding Object to be
GCd
    
There could be more than one MockFaceDetection association with the same
MockFaceDetectionProvider, so Holding the MockFaceDetection to prevent
Binding Object to be GCd when calling createFaceDetection function frequently.
    
BUG=707689
==========

to

==========
ShapeDetection: Make sure tests are running serially
    
There could be more than one MockFaceDetection association with the same
MockFaceDetectionProvider, the first mock service is random Gcd in same
context, so ensure these tests run serially.
    
BUG=707689
==========

[Ken Rockot(use gerrit already), 2017-05-27 06:23:52]

As I discovered just recently, you can use promise_test instead of async_test,
and this will ensure that each test runs serially. I think that may be
preferable to combining everything into a single test. WDYT?

[junwei, 2017-05-27 07:37:00]

On 2017/05/27 06:23:52, Ken Rockot(use gerrit already) wrote:
> As I discovered just recently, you can use promise_test instead of async_test,
> and this will ensure that each test runs serially. I think that may be
> preferable to combining everything into a single test. WDYT?

The generate_tests will generate 6 tests which are parallel even though the test
internal is promise, so promise_test doesn't ensure outside tests run serially.

[Ken Rockot(use gerrit already), 2017-05-27 07:52:01]

On 2017/05/27 at 07:37:00, junwei.fu wrote:
> On 2017/05/27 06:23:52, Ken Rockot(use gerrit already) wrote:
> > As I discovered just recently, you can use promise_test instead of
async_test,
> > and this will ensure that each test runs serially. I think that may be
> > preferable to combining everything into a single test. WDYT?
> 
> The generate_tests will generate 6 tests which are parallel even though the
test internal is promise, so promise_test doesn't ensure outside tests run
serially.

Maybe I'm looking at the wrong code, but it seems pretty obvious to me that
these tests use async_test and not promise_test. generate_tests is using
createTestForCanvasElement as the generator, and that createTestForCanvasElement
very clearly calls async_test. async_test does not use promise_test internally.

I'm reading the code for promise_test and it appears to be designed to run tests
serially in a queue. I have tested this locally and confirmed that it's
accurate.

[junwei, 2017-05-27 08:13:21]

On 2017/05/27 07:52:01, Ken Rockot(use gerrit already) wrote:
> On 2017/05/27 at 07:37:00, junwei.fu wrote:
> > On 2017/05/27 06:23:52, Ken Rockot(use gerrit already) wrote:
> > > As I discovered just recently, you can use promise_test instead of
> async_test,
> > > and this will ensure that each test runs serially. I think that may be
> > > preferable to combining everything into a single test. WDYT?
> > 
> > The generate_tests will generate 6 tests which are parallel even though the
> test internal is promise, so promise_test doesn't ensure outside tests run
> serially.
> 
> Maybe I'm looking at the wrong code, but it seems pretty obvious to me that
> these tests use async_test and not promise_test. generate_tests is using
> createTestForCanvasElement as the generator, and that
createTestForCanvasElement
> very clearly calls async_test. async_test does not use promise_test
internally.
> 
> I'm reading the code for promise_test and it appears to be designed to run
tests
> serially in a queue. I have tested this locally and confirmed that it's
> accurate.
Yes, the internal promise_test is run serially, but two promise_tests are run at
the same time.
[1] is documentation of using promise_test and async_test.

[1] http://web-platform-tests.org/writing-tests/testharness-api.html

[Ken Rockot(use gerrit already), 2017-05-27 16:45:51]

Quoting from that link:

Unlike Asynchronous Tests, Promise Tests don’t start running until after
the previous Promise Test finishes.

On May 27, 2017 1:13 AM, <junwei.fu@intel.com> wrote:

> On 2017/05/27 07:52:01, Ken Rockot(use gerrit already) wrote:
> > On 2017/05/27 at 07:37:00, junwei.fu wrote:
> > > On 2017/05/27 06:23:52, Ken Rockot(use gerrit already) wrote:
> > > > As I discovered just recently, you can use promise_test instead of
> > async_test,
> > > > and this will ensure that each test runs serially. I think that may
> be
> > > > preferable to combining everything into a single test. WDYT?
> > >
> > > The generate_tests will generate 6 tests which are parallel even
> though the
> > test internal is promise, so promise_test doesn't ensure outside tests
> run
> > serially.
> >
> > Maybe I'm looking at the wrong code, but it seems pretty obvious to me
> that
> > these tests use async_test and not promise_test. generate_tests is using
> > createTestForCanvasElement as the generator, and that
> createTestForCanvasElement
> > very clearly calls async_test. async_test does not use promise_test
> internally.
> >
> > I'm reading the code for promise_test and it appears to be designed to
> run
> tests
> > serially in a queue. I have tested this locally and confirmed that it's
> > accurate.
> Yes, the internal promise_test is run serially, but two promise_tests are
> run at
> the same time.
> [1] is documentation of using promise_test and async_test.
>
> [1] http://web-platform-tests.org/writing-tests/testharness-api.html
>
> https://codereview.chromium.org/2818563006/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2017-05-27 16:45:53]

Quoting from that link:

Unlike Asynchronous Tests, Promise Tests don’t start running until after
the previous Promise Test finishes.

On May 27, 2017 1:13 AM, <junwei.fu@intel.com> wrote:

> On 2017/05/27 07:52:01, Ken Rockot(use gerrit already) wrote:
> > On 2017/05/27 at 07:37:00, junwei.fu wrote:
> > > On 2017/05/27 06:23:52, Ken Rockot(use gerrit already) wrote:
> > > > As I discovered just recently, you can use promise_test instead of
> > async_test,
> > > > and this will ensure that each test runs serially. I think that may
> be
> > > > preferable to combining everything into a single test. WDYT?
> > >
> > > The generate_tests will generate 6 tests which are parallel even
> though the
> > test internal is promise, so promise_test doesn't ensure outside tests
> run
> > serially.
> >
> > Maybe I'm looking at the wrong code, but it seems pretty obvious to me
> that
> > these tests use async_test and not promise_test. generate_tests is using
> > createTestForCanvasElement as the generator, and that
> createTestForCanvasElement
> > very clearly calls async_test. async_test does not use promise_test
> internally.
> >
> > I'm reading the code for promise_test and it appears to be designed to
> run
> tests
> > serially in a queue. I have tested this locally and confirmed that it's
> > accurate.
> Yes, the internal promise_test is run serially, but two promise_tests are
> run at
> the same time.
> [1] is documentation of using promise_test and async_test.
>
> [1] http://web-platform-tests.org/writing-tests/testharness-api.html
>
> https://codereview.chromium.org/2818563006/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[junwei, 2017-05-29 05:43:46]

On 2017/05/27 16:45:53, Ken Rockot(use gerrit already) wrote:
> Quoting from that link:
> 
> Unlike Asynchronous Tests, Promise Tests don’t start running until after
> the previous Promise Test finishes.
> 
> On May 27, 2017 1:13 AM, <mailto:junwei.fu@intel.com> wrote:
> 
> > On 2017/05/27 07:52:01, Ken Rockot(use gerrit already) wrote:
> > > On 2017/05/27 at 07:37:00, junwei.fu wrote:
> > > > On 2017/05/27 06:23:52, Ken Rockot(use gerrit already) wrote:
> > > > > As I discovered just recently, you can use promise_test instead of
> > > async_test,
> > > > > and this will ensure that each test runs serially. I think that may
> > be
> > > > > preferable to combining everything into a single test. WDYT?
> > > >
> > > > The generate_tests will generate 6 tests which are parallel even
> > though the
> > > test internal is promise, so promise_test doesn't ensure outside tests
> > run
> > > serially.
> > >
> > > Maybe I'm looking at the wrong code, but it seems pretty obvious to me
> > that
> > > these tests use async_test and not promise_test. generate_tests is using
> > > createTestForCanvasElement as the generator, and that
> > createTestForCanvasElement
> > > very clearly calls async_test. async_test does not use promise_test
> > internally.
> > >
> > > I'm reading the code for promise_test and it appears to be designed to
> > run
> > tests
> > > serially in a queue. I have tested this locally and confirmed that it's
> > > accurate.
> > Yes, the internal promise_test is run serially, but two promise_tests are
> > run at
> > the same time.
> > [1] is documentation of using promise_test and async_test.
> >
> > [1] http://web-platform-tests.org/writing-tests/testharness-api.html
> >
> > https://codereview.chromium.org/2818563006/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

My original implementation with promise_test is hold 'detectShapeForCanvas' in
'then' section that is still run concurrently.
Patch Set 6 is ensure run serially with promise_test as expected, PTAL, thanks.

[Ken Rockot(use gerrit already), 2017-05-29 06:12:55]

Tentatively LGTM if the tests pass. Thanks and sorry about the need for all the
back-and-forth.

This can be simplified further once we have the new bindings ready for use (very
soon).

https://codereview.chromium.org/2818563006/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js
(right):

https://codereview.chromium.org/2818563006/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js:23:
this.mock_service_ = new MockFaceDetection(request, options);
nit: mockService_?

[junwei, 2017-05-29 06:28:29]

The CQ bit was checked by junwei.fu@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-29 06:28:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[junwei, 2017-05-29 07:16:04]

Patchset #7 (id:240001) has been deleted

[junwei, 2017-05-29 07:16:12]

The CQ bit was checked by junwei.fu@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-29 07:16:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-29 08:31:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-29 08:31:10]

Dry run: This issue passed the CQ dry run.

[junwei, 2017-05-29 11:05:35]

The CQ bit was checked by junwei.fu@intel.com

[junwei, 2017-05-29 11:05:35]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org,
rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2818563006/#ps260001
(title: "ShapeDetection: Make sure tests are running serially")

[commit-bot: I haz the power, 2017-05-29 11:05:42]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-29 11:09:52]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1496055935249870,
"parent_rev": "2a97a5d29f8b0518066e152d9f87ba5700954796", "commit_rev":
"3324464e62044d7d8cfdac18bac1fcb2835cc6ff"}

[commit-bot: I haz the power, 2017-05-29 11:10:06]

Description was changed from

==========
ShapeDetection: Make sure tests are running serially
    
There could be more than one MockFaceDetection association with the same
MockFaceDetectionProvider, the first mock service is random Gcd in same
context, so ensure these tests run serially.
    
BUG=707689
==========

to

==========
ShapeDetection: Make sure tests are running serially
    
There could be more than one MockFaceDetection association with the same
MockFaceDetectionProvider, the first mock service is random Gcd in same
context, so ensure these tests run serially.
    
BUG=707689

Review-Url: https://codereview.chromium.org/2818563006
Cr-Commit-Position: refs/heads/master@{#475335}
Committed:
https://chromium.googlesource.com/chromium/src/+/3324464e62044d7d8cfdac18bac1...
==========

[commit-bot: I haz the power, 2017-05-29 11:10:07]

Committed patchset #7 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/3324464e62044d7d8cfdac18bac1...

[junwei, 2017-05-29 11:13:58]

On 2017/05/29 06:12:55, Ken Rockot(use gerrit already) wrote:
> Tentatively LGTM if the tests pass. Thanks and sorry about the need for all
the
> back-and-forth.
> 
> This can be simplified further once we have the new bindings ready for use
(very
> soon).
I'm pleasure to do anythings, please let me know if you need.

>
https://codereview.chromium.org/2818563006/diff/220001/third_party/WebKit/Lay...
> File
> third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js
> (right):
> 
>
https://codereview.chromium.org/2818563006/diff/220001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/shapedetection/resources/mock-facedetection.js:23:
> this.mock_service_ = new MockFaceDetection(request, options);
> nit: mockService_?
Done.