Re: Addressing FIXME in WebSocket.idl (issue 871013007 by paritosh.in@samsung.com)

[parit...@samsung.com]

As now we are logging in binding code for invalid enum type, as
https://codereview.chromium.org/955413002/ does this.

Updated CL after https://codereview.chromium.org/955413002/.

PTAL.

https://codereview.chromium.org/871013007/

[tyos...@chromium.org]

lgtm

Thanks!

https://codereview.chromium.org/871013007/

[tyos...@chromium.org]

Please update the description to explain what you did than saying "some
FIXME is
addressed" before committing

https://codereview.chromium.org/871013007/

[parit...@samsung.com]

On 2015/03/30 08:13:01, tyoshino wrote:
> Please update the description to explain what you did than saying "some
> FIXME
is
> addressed" before committing

Thanks. Updated the description.

https://codereview.chromium.org/871013007/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/871013007/40001


https://codereview.chromium.org/871013007/

[commi...@chromium.org]

Try jobs failed on following builders:
linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/54827)

https://codereview.chromium.org/871013007/

[nba...@chromium.org]

On 2015/03/30 08:33:12, Paritosh Kumar wrote:
> Thanks. Updated the description.

In future, could you also make sure the *Title* is descriptive?
I've fixed it here, so it's
"Use enum BinaryType for WebSocket.binaryType, instead of DOMString"
instead of
"Addressing FIXME in WebSocket.idl"

Thanks for seeing this through!

https://codereview.chromium.org/871013007/

[parit...@samsung.com]

Thanks Nils

> In future, could you also make sure the *Title* is descriptive?

Sure.

> I've fixed it here, so it's
> "Use enum BinaryType for WebSocket.binaryType, instead of DOMString"
> instead of
> "Addressing FIXME in WebSocket.idl"

Thanks..


As we are using ASSERT_NOT_REACHED() in DOMWebSocket::binaryType()
This results in failure of DOMWebSocketTest.binaryType Test as
we are seeing it in
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/54827/steps/webkit_unit_tests/logs/stdio

Actually, This test doesn't hit the ASSERT in Release mode, but Hits in
Debug
mode of webkit_unit_tests, so didn't identified.


https://codereview.chromium.org/871013007/

[nba...@chromium.org]

On 2015/03/30 13:10:10, Paritosh Kumar wrote:
> As we are using ASSERT_NOT_REACHED() in DOMWebSocket::binaryType()
> This results in failure of DOMWebSocketTest.binaryType Test as
> we are seeing it in

http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/54827/steps/webkit_unit_tests/logs/stdio

> Actually, This test doesn't hit the ASSERT in Release mode, but Hits in
> Debug
> mode of webkit_unit_tests, so didn't identified.

Could you fix these failures by updating the tests?

Specifically, could you fix DOMWebSocketTest.binaryType in:
modules/websockets/DOMWebSocketTest.cpp
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/modules/websockets/DOMWebSocketTest.cpp&q=file:DOMWebSocketTest&sq=package:chromium&l=1

The lines:
m_websocket->setBinaryType("hoge");
// ...
m_websocket->setBinaryType("fuga");
...are causing the crash, and such calls should be prevented by the bindings
code;
this is a precondition violation.

Thus, could you replace the EXPECT_EQ with EXPECT_FATAL_FAILURE, as:
EXPECT_FATAL_FAILURE(m_websocket->setBinaryType("hoge"), "");
// ...
EXPECT_FATAL_FAILURE(m_websocket->setBinaryType("fuga"), "");

You'll need:
#include <gtest/gtest-spi.h>

See:
https://code.google.com/p/googletest/wiki/AdvancedGuide#Catching_Failures

https://codereview.chromium.org/871013007/

[nba...@chromium.org]

On 2015/03/30 13:10:10, Paritosh Kumar wrote:
> Actually, This test doesn't hit the ASSERT in Release mode, but Hits in
> Debug
> mode of webkit_unit_tests, so didn't identified.

To be precise:
It *did* hit the ASSERT line, but ASSERT_* statements are compiled *out* of
Release builds.

https://codereview.chromium.org/871013007/

[parit...@samsung.com]

Thanks Nils

> Thus, could you replace the EXPECT_EQ with EXPECT_FATAL_FAILURE, as:
> EXPECT_FATAL_FAILURE(m_websocket->setBinaryType("hoge"), "");
> // ...
> EXPECT_FATAL_FAILURE(m_websocket->setBinaryType("fuga"), "");

But, we cannot use non-static member m_websocket in this macro.
according to:
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/include/gtest/gtest-spi.h&q=EXPECT_FATAL_FAILURE&sq=package:chromium&l=128

> To be precise:
> It *did* hit the ASSERT line, but ASSERT_* statements are compiled *out*
> of
> Release builds.

Ohhkk.. Thanks.


https://codereview.chromium.org/871013007/

[nba...@chromium.org]

On 2015/03/31 06:51:57, Paritosh Kumar wrote:
> > Thus, could you replace the EXPECT_EQ with EXPECT_FATAL_FAILURE, as:
> > EXPECT_FATAL_FAILURE(m_websocket->setBinaryType("hoge"), "");
> > // ...
> > EXPECT_FATAL_FAILURE(m_websocket->setBinaryType("fuga"), "");

> But, we cannot use non-static member m_websocket in this macro.
> according to:

https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/include/gtest/gtest-spi.h&q=EXPECT_FATAL_FAILURE&sq=package:chromium&l=128

Oops, sorry, wrong assert/expect.
Could you try using EXPECT_DEATH instead?
https://code.google.com/p/googletest/wiki/AdvancedGuide
https://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests

https://codereview.chromium.org/871013007/

[parit...@samsung.com]

Thanks Nils

> Could you try using EXPECT_DEATH instead?
> https://code.google.com/p/googletest/wiki/AdvancedGuide
> https://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests

As ASSERT_* statements are compiled *out* of Release builds. I think, We
can't
expect
CRASH on setting invalid binaryType in DOMWebSocketTest, for Release build
also.
and use
of EXPECT_DEATH is resulting in failure of DOMWebSocketTest.binaryType
Test, so
using
EXPECT_DEBUG_DEATH instead.

Please have a look.

Thanks & Regards

https://codereview.chromium.org/871013007/

[nba...@chromium.org]

Thanks Paritosh, that's correct.
LGTM!

https://codereview.chromium.org/871013007/

[commi...@chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/patch-status/871013007/60001


https://codereview.chromium.org/871013007/

[parit...@samsung.com]

Thanks.

https://codereview.chromium.org/871013007/

[parit...@samsung.com]

Still, CRASH occurs, as we can see, It fails for Release build only
according to
build command
"/b/build/slave/linux_layout/build/src/out/Release/webkit_unit_tests".

So, I think we should use EXPECT_DEATH instead of EXPEXT_DEBUG_DEATH. Am I
right?


https://codereview.chromium.org/871013007/

[nba...@chromium.org]

That sounds right, thanks for checking!
Also, could you move the EXPECT_DEATH statements to a separate test case
called
DOMWebSocketDeathTest (to avoid threading issues) as:

TEST_F(DOMWebSocketDeathTest, binaryType)
...per:
"Important: We strongly recommend you to follow the convention of naming
your
test case (not test) *DeathTest when it contains a death test, as
demonstrated
in the above example."
https://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests_And_Threads

https://codereview.chromium.org/871013007/

[parit...@samsung.com]

Thanks. Moved the *DeathTest to separate Test case.
Using EXPECT_DEATH for CRASH.

PTAL.

https://codereview.chromium.org/871013007/

[commi...@chromium.org]

Dry run: CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/871013007/80001


https://codereview.chromium.org/871013007/

[commi...@chromium.org]

[nba...@chromium.org]

Thanks, still LGTM (with nits).


https://codereview.chromium.org/871013007/diff/80001/Source/modules/websockets/DOMWebSocketTest.cpp
File Source/modules/websockets/DOMWebSocketTest.cpp (right):

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websockets/DOMWebSocketTest.cpp#newcode668
Source/modules/websockets/DOMWebSocketTest.cpp:668:
BTW, could you remove these blank lines?
They're useless and nowhere else in this test uses them.

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websockets/DOMWebSocketTest.cpp#newcode685
Source/modules/websockets/DOMWebSocketTest.cpp:685:
Same for these lines.

https://codereview.chromium.org/871013007/

[parit...@samsung.com]

Thanks.
As we can see, the result of cq-dry-run is failing test
http/tests/websocket/binary-type.html

I didn't get any reason of this result, as this should be prevented by
binding
code.

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websockets/DOMWebSocketTest.cpp
File Source/modules/websockets/DOMWebSocketTest.cpp (right):

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websockets/DOMWebSocketTest.cpp#newcode668
Source/modules/websockets/DOMWebSocketTest.cpp:668:

On 2015/04/02 14:56:35, Nils Barth (inactive) wrote:
> BTW, could you remove these blank lines?
> They're useless and nowhere else in this test uses them.

I think, this space is between one operation call and EXPECT_* macro.

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websockets/DOMWebSocketTest.cpp#newcode689
Source/modules/websockets/DOMWebSocketTest.cpp:689:
We can remove this space.

https://codereview.chromium.org/871013007/

[nba...@chromium.org]

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websockets/DOMWebSocketTest.cpp
File Source/modules/websockets/DOMWebSocketTest.cpp (right):

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websockets/DOMWebSocketTest.cpp#newcode668
Source/modules/websockets/DOMWebSocketTest.cpp:668:

On 2015/04/02 15:41:32, Paritosh Kumar wrote:
> On 2015/04/02 14:56:35, Nils Barth (inactive) wrote:
> > BTW, could you remove these blank lines?
> > They're useless and nowhere else in this test uses them.

> I think, this space is between one operation call and EXPECT_* macro.

Ok, point taken.
(It's a bit loose spacing for my taste, but ok to leave as is.)

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websockets/DOMWebSocketTest.cpp#newcode689
Source/modules/websockets/DOMWebSocketTest.cpp:689:

On 2015/04/02 15:41:32, Paritosh Kumar wrote:
> We can remove this space.

Good point; could you? Thanks!

https://codereview.chromium.org/871013007/

[parit...@samsung.com]

Thanks Nils

Addressed your comments.

Please have a look.

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websockets/DOMWebSocketTest.cpp
File Source/modules/websockets/DOMWebSocketTest.cpp (right):

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websockets/DOMWebSocketTest.cpp#newcode668
Source/modules/websockets/DOMWebSocketTest.cpp:668:

On 2015/04/02 15:54:55, Nils Barth (inactive) wrote:
> On 2015/04/02 15:41:32, Paritosh Kumar wrote:
> > On 2015/04/02 14:56:35, Nils Barth (inactive) wrote:
> > > BTW, could you remove these blank lines?
> > > They're useless and nowhere else in this test uses them.
> >
> > I think, this space is between one operation call and EXPECT_*
macro.

> Ok, point taken.
> (It's a bit loose spacing for my taste, but ok to leave as is.)

Thanks. Leaving this space.

https://codereview.chromium.org/871013007/diff/80001/Source/modules/websockets/DOMWebSocketTest.cpp#newcode689
Source/modules/websockets/DOMWebSocketTest.cpp:689:

On 2015/04/02 15:54:55, Nils Barth (inactive) wrote:
> On 2015/04/02 15:41:32, Paritosh Kumar wrote:
> > We can remove this space.

> Good point; could you? Thanks!

Thanks Removed.

https://codereview.chromium.org/871013007/

[nba...@chromium.org]

On 2015/04/02 15:41:32, Paritosh Kumar wrote:

> Thanks.
> As we can see, the result of cq-dry-run is failing test
> http/tests/websocket/binary-type.html

> I didn't get any reason of this result, as this should be prevented by
> binding
> code.

Thanks Paritosh!
Could you debug this failures?
(Try running the tests locally?)

https://codereview.chromium.org/871013007/

[parit...@samsung.com]

On 2015/04/02 16:20:39, Nils Barth (inactive) wrote:
> On 2015/04/02 15:41:32, Paritosh Kumar wrote:
> > Thanks.
> > As we can see, the result of cq-dry-run is failing test
> > http/tests/websocket/binary-type.html
> >
> > I didn't get any reason of this result, as this should be prevented by
binding
> > code.

> Thanks Paritosh!
> Could you debug this failures?
> (Try running the tests locally?)

Yes, I ran it locally, its working fine and no failure found.

BTW, binding code will prevent any call to setBinaryType() with wrong
value of binaryType. and It won't hit ASSERT line in setBinaryType().

I think, CRASH occurs due to hitting that ASSERT.(may be other cause?)


https://codereview.chromium.org/871013007/

[nba...@chromium.org]

On 2015/04/02 17:04:18, Paritosh Kumar wrote:
> On 2015/04/02 16:20:39, Nils Barth (inactive) wrote:
> > On 2015/04/02 15:41:32, Paritosh Kumar wrote:
> > > Thanks.
> > > As we can see, the result of cq-dry-run is failing test
> > > http/tests/websocket/binary-type.html
> > >
> > > I didn't get any reason of this result, as this should be prevented by
> binding
> > > code.
> >
> > Thanks Paritosh!
> > Could you debug this failures?
> > (Try running the tests locally?)

> Yes, I ran it locally, its working fine and no failure found.

Could you run the test in Debug mode?
(If still nothing, could you ask active developers for help?)

I think the command is:
Tools/Scripts/run-webkit-tests --debug http/tests/websocket/binary-type.html

https://codereview.chromium.org/871013007/

[jsb...@chromium.org]

I believe this is now failing due to
https://codereview.chromium.org/1047993002

Paritosh: if you sync to the latest code and re-run you should see the
failure.

That CL - which just landed a few hours ago - added support for enum
validation
in sequences/arrays. But it seems to have undone some of the work by
Paritosh.
For example:

https://codereview.chromium.org/1047993002/diff/100001/LayoutTests/webaudio/pannernode-basic-expected.txt

it replaces this nice error:

CONSOLE WARNING: The provided value '1' is not a valid value of type
'PanningModelType'.

with this generic one:

CONSOLE WARNING: The provided value '1' is not a valid enum value.

bashi@ - can we get the nice errors back?


https://codereview.chromium.org/871013007/

[jsb...@chromium.org]

FWIW, I suggest that Paritosh updated the -expected.txt file in this CL to
match
the generic output ("not a valid enum value"), then land this CL. As a
follow-up
we should work with bashi to explore restoring the more detailed enum
logging
that Paritosh worked so hard to get in.

The review comments in https://codereview.chromium.org/1047993002 explain
why
the change was made as a simplifying change, but it's a shame to lose it
given
the context here and also https://codereview.chromium.org/955413002/

https://codereview.chromium.org/871013007/

[nba...@chromium.org]

Thanks for details Joshua!

Paritosh, per suggestion, could you sync, update, and land this CL, then
follow
up?

https://codereview.chromium.org/871013007/

[ba...@chromium.org]

Sure. I'll create a CL.

https://codereview.chromium.org/871013007/

[nba...@chromium.org]

On 2015/04/02 17:38:36, jsbell wrote:
> > bashi@ - can we get the nice errors back?

On 2015/04/03 01:35:10, bashi1 wrote:
> Sure. I'll create a CL.

Thanks Bashi!

Paritosh, feel free to sync, update, and land this CL;
Bashi can handle the updates ;)

https://codereview.chromium.org/871013007/

[parit...@samsung.com]

On 2015/04/03 14:41:46, Nils Barth (inactive) wrote:
> On 2015/04/02 17:38:36, jsbell wrote:
> > > bashi@ - can we get the nice errors back?

> On 2015/04/03 01:35:10, bashi1 wrote:
> > Sure. I'll create a CL.

> Thanks Bashi!

> Paritosh, feel free to sync, update, and land this CL;
> Bashi can handle the updates ;)

Okay, Thanks. I'll update it.

https://codereview.chromium.org/871013007/

[parit...@samsung.com]

On 2015/04/03 16:45:22, Paritosh Kumar wrote:
> On 2015/04/03 14:41:46, Nils Barth (inactive) wrote:
> > On 2015/04/02 17:38:36, jsbell wrote:
> > > > bashi@ - can we get the nice errors back?
> >
> > On 2015/04/03 01:35:10, bashi1 wrote:
> > > Sure. I'll create a CL.
> >
> > Thanks Bashi!
> >
> > Paritosh, feel free to sync, update, and land this CL;
> > Bashi can handle the updates ;)

> Okay, Thanks. I'll update it.

Running CQ dry run..

https://codereview.chromium.org/871013007/

[commi...@chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/patch-status/871013007/120001


https://codereview.chromium.org/871013007/

[commi...@chromium.org]

[commi...@chromium.org]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193201

https://codereview.chromium.org/871013007/

[jsb...@chromium.org]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1053013006/ by jsb...@chromium.org.

The reason for reverting is: Failing in gtests:

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.8/builds/35898/steps/webkit_unit_tests/logs/DOMWebSocketDeathTest.binaryType

[WARNING] ../../testing/gtest/src/gtest-death-test.cc:825:: Death tests use
fork(), which is unsafe particularly in a threaded context. For this test,
Google Test detected 5 threads.
../../third_party/WebKit/Source/modules/websockets/DOMWebSocketTest.cpp:687:
Failure
Death test: m_websocket->setBinaryType("hoge")
Result: failed to die.
Error msg:
[ DEATH ]
[ FAILED ] DOMWebSocketDeathTest.binaryType (5 ms)
.

https://codereview.chromium.org/871013007/

[nba...@chromium.org]

Thanks Joshua!
Looks like we can't use death tests.
Paritosh, could you remove the death test and resubmit?
Thanks!

https://codereview.chromium.org/871013007/