Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8400024:
Add TestConnectFailure, TestGetHandleFailure and TestConnectAndPipe to PPAPI Broker UI test. (Closed)
DescriptionAdd TestConnectFailure, TestGetHandleFailure and TestConnectAndPipe to PPAPI Broker UI test.
Also fix wrong sandbox options for broker process on Windows and Mac.
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110241
Patch Set 1 #
Total comments: 27
Patch Set 2 : Update according to the comments. #
Total comments: 12
Patch Set 3 : Added SyncSocket for platform compatibility. Also made changes based on comments. #
Total comments: 10
Patch Set 4 : Modified as commented. #
Total comments: 5
Patch Set 5 : Modified as comment advised. #Patch Set 6 : Add sandbox fix for broker on Win/Mac. #Patch Set 7 : Resync to solve patching error. #Patch Set 8 : Rebased. Accroding to 3f364cbe2565b02, added a string parameter to RunTests(). #Patch Set 9 : Removed dependency on base. Test passed on Linux. Need to test on Windows and Mac. #Patch Set 10 : Remove dependencies on "base". #Patch Set 11 : Disable TestConnectAndPipe on Windows due to a bug on Win. #
Total comments: 30
Patch Set 12 : Modify based on code review comments. #
Total comments: 2
Patch Set 13 : Modify as commented. #
Total comments: 8
Patch Set 14 : Modified as commented. #
Total comments: 2
Patch Set 15 : Removed unnecessary fdopen as commented by piman. #Patch Set 16 : Change #include "windows.h" to <windows.h> #Patch Set 17 : Further removed all dependencies on src/base. #Patch Set 18 : Remove the paranoid undef's. #Patch Set 19 : Add !! before ::DeleteFile since BOOL is not bool. #
Messages
Total messages: 30 (0 generated)
Looks good overall. Mostly nits. http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc File ppapi/tests/test_broker.cc (right): http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:21: const size_t kTestMessageLength = 30; use arraysize() instead? http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:23: const int32_t kTestSandboxSuccess = 0; kBrokerIsUnsandboxed I would write a string since 0 and 1 could occur on the pipe for other reasons. http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:27: bool TestSandbox(bool is_sandbox_expected) { VerifyIsUnsandboxed Since we're only testing it is unsandboxed, we don't need is_sandbox_expected (always false). http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:31: // FIXME: Add sandbox test for other platforms. Move to an #else below and change to "Windows". Actually, see http://codesearch.google.com/#OAMlx_jo-ck/src/webkit/plugins/ppapi/ppb_buffer.... Actually, since this just uses an fstream, all we need to do is change the paths. http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:33: const char* const kWriteFile = "/tmp/broker_sandbox_test.txt"; /tmp/ppapi_... Paths should be the only difference. Move these up with the other constants and add appropriate Mac and Win ifdefs. http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:40: const char* const kReadFile = "/var/log/messages"; messages does not exist on my Mac. kernel.log might be a reasonable replacement. You can use #if defined(OS_MACOSX) to select this. Move up with the write file. http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:46: fs << "this will not work"; Probably not necessary. http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:110: There is more testing of Connect and the handle that could be done here. http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:114: std::string TestBroker::TestConnect() { ...AndGetHandle http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:115: ASSERT_TRUE(broker_interface_); I think this is an assumption in all tests, so we probably don't need it. http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:122: ASSERT_EQ(PP_OK_COMPLETIONPENDING, broker_interface_->Connect( I'm not sure why the existing test uses ASSERT, but EXPECT is usually better unless a failure will cause a crash in the test. http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:126: int32_t handle = -1; Can we use PlatformFileToInt(base::kInvalidPlatformFileValue)? See http://codesearch.google.com/#OAMlx_jo-ck/src/webkit/plugins/ppapi/ppb_broker.... You can then EXPECT_NE(PlatformFileToInt(base::kInvalidPlatformFileValue), handle); below. http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:131: ASSERT_EQ(0, ::strcmp(received_msg.get(), kTestMessage)); EXPECT_STREQ
Mostly done. Don't quite understand this one: "> There is more testing of Connect and the handle that could be done here." Also EXPECT* is not available in this test. http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc File ppapi/tests/test_broker.cc (right): http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:21: const size_t kTestMessageLength = 30; On 2011/10/27 18:19:46, ddorwin wrote: > use arraysize() instead? Done. http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:23: const int32_t kTestSandboxSuccess = 0; On 2011/10/27 18:19:46, ddorwin wrote: > kBrokerIsUnsandboxed > > I would write a string since 0 and 1 could occur on the pipe for other reasons. Done. http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:27: bool TestSandbox(bool is_sandbox_expected) { On 2011/10/27 18:19:46, ddorwin wrote: > VerifyIsUnsandboxed > > Since we're only testing it is unsandboxed, we don't need is_sandbox_expected > (always false). Done. http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:27: bool TestSandbox(bool is_sandbox_expected) { On 2011/10/27 18:19:46, ddorwin wrote: > VerifyIsUnsandboxed > > Since we're only testing it is unsandboxed, we don't need is_sandbox_expected > (always false). Done. http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:31: // FIXME: Add sandbox test for other platforms. On 2011/10/27 18:19:46, ddorwin wrote: > Move to an #else below and change to "Windows". Actually, see > http://codesearch.google.com/#OAMlx_jo-ck/src/webkit/plugins/ppapi/ppb_buffer.... > > Actually, since this just uses an fstream, all we need to do is change the > paths. Done. http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:33: const char* const kWriteFile = "/tmp/broker_sandbox_test.txt"; On 2011/10/27 18:19:46, ddorwin wrote: > /tmp/ppapi_... > > Paths should be the only difference. Move these up with the other constants and > add appropriate Mac and Win ifdefs. Done. http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:40: const char* const kReadFile = "/var/log/messages"; On 2011/10/27 18:19:46, ddorwin wrote: > messages does not exist on my Mac. kernel.log might be a reasonable replacement. > You can use #if defined(OS_MACOSX) to select this. > > Move up with the write file. Done. http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:46: fs << "this will not work"; On 2011/10/27 18:19:46, ddorwin wrote: > Probably not necessary. Done. http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:110: On 2011/10/27 18:19:46, ddorwin wrote: > There is more testing of Connect and the handle that could be done here. ?? Don't get it... http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:114: std::string TestBroker::TestConnect() { On 2011/10/27 18:19:46, ddorwin wrote: > ...AndGetHandle Done. http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:115: ASSERT_TRUE(broker_interface_); On 2011/10/27 18:19:46, ddorwin wrote: > I think this is an assumption in all tests, so we probably don't need it. Done. http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:122: ASSERT_EQ(PP_OK_COMPLETIONPENDING, broker_interface_->Connect( On 2011/10/27 18:19:46, ddorwin wrote: > I'm not sure why the existing test uses ASSERT, but EXPECT is usually better > unless a failure will cause a crash in the test. EXPECT is no available :-( http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:126: int32_t handle = -1; On 2011/10/27 18:19:46, ddorwin wrote: > Can we use PlatformFileToInt(base::kInvalidPlatformFileValue)? > See > http://codesearch.google.com/#OAMlx_jo-ck/src/webkit/plugins/ppapi/ppb_broker.... > > You can then EXPECT_NE(PlatformFileToInt(base::kInvalidPlatformFileValue), > handle); below. Done.
This won't compile on Windows. I think we can actually make it pass on Windows with SyncSocket. http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc File ppapi/tests/test_broker.cc (right): http://codereview.chromium.org/8400024/diff/1/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:110: On 2011/10/27 22:31:57, xhwang wrote: > On 2011/10/27 18:19:46, ddorwin wrote: > > There is more testing of Connect and the handle that could be done here. > > ?? Don't get it... As discussed. http://codereview.chromium.org/8400024/diff/4001/ppapi/tests/test_broker.cc File ppapi/tests/test_broker.cc (right): http://codereview.chromium.org/8400024/diff/4001/ppapi/tests/test_broker.cc#n... ppapi/tests/test_broker.cc:26: // FIXME: merge all PlatformFileToInt definitions and move to a common place. Chrome uses TODO(id). WebKit uses FIXME. http://codereview.chromium.org/8400024/diff/4001/ppapi/tests/test_broker.cc#n... ppapi/tests/test_broker.cc:46: temp_dir_.path().Append(FILE_PATH_LITERAL("verify_sandbox.txt")); indent 4. verify implies it should be sandboxed. Maybe test_pepper_broker.txt. http://codereview.chromium.org/8400024/diff/4001/ppapi/tests/test_broker.cc#n... ppapi/tests/test_broker.cc:49: if (!fs) return false; While it makes the code harder to read, these should all be 2 lines. http://codereview.chromium.org/8400024/diff/4001/ppapi/tests/test_broker.cc#n... ppapi/tests/test_broker.cc:50: fs << "Verify if the broker is unsandboxed."; if => that http://codereview.chromium.org/8400024/diff/4001/ppapi/tests/test_broker.cc#n... ppapi/tests/test_broker.cc:66: ::read(handle, message_received.get(), length); This part is OS-specific. You might be able to use http://codesearch.google.com/#OAMlx_jo-ck/src/base/sync_socket.h&type=cs to avoid this. http://codereview.chromium.org/8400024/diff/4001/ppapi/tests/test_broker.cc#n... ppapi/tests/test_broker.cc:144: int32_t handle = -1; = PlatformFileToInt(base::kInvalidPlatformFileValue)
Done as commented. Thanks for the great review! http://codereview.chromium.org/8400024/diff/4001/ppapi/tests/test_broker.cc File ppapi/tests/test_broker.cc (right): http://codereview.chromium.org/8400024/diff/4001/ppapi/tests/test_broker.cc#n... ppapi/tests/test_broker.cc:26: // FIXME: merge all PlatformFileToInt definitions and move to a common place. On 2011/10/27 23:21:24, ddorwin wrote: > Chrome uses TODO(id). WebKit uses FIXME. Done. http://codereview.chromium.org/8400024/diff/4001/ppapi/tests/test_broker.cc#n... ppapi/tests/test_broker.cc:46: temp_dir_.path().Append(FILE_PATH_LITERAL("verify_sandbox.txt")); On 2011/10/27 23:21:24, ddorwin wrote: > indent 4. > verify implies it should be sandboxed. Maybe test_pepper_broker.txt. Done. http://codereview.chromium.org/8400024/diff/4001/ppapi/tests/test_broker.cc#n... ppapi/tests/test_broker.cc:49: if (!fs) return false; On 2011/10/27 23:21:24, ddorwin wrote: > While it makes the code harder to read, these should all be 2 lines. Done. http://codereview.chromium.org/8400024/diff/4001/ppapi/tests/test_broker.cc#n... ppapi/tests/test_broker.cc:50: fs << "Verify if the broker is unsandboxed."; On 2011/10/27 23:21:24, ddorwin wrote: > if => that Done. http://codereview.chromium.org/8400024/diff/4001/ppapi/tests/test_broker.cc#n... ppapi/tests/test_broker.cc:66: ::read(handle, message_received.get(), length); On 2011/10/27 23:21:24, ddorwin wrote: > This part is OS-specific. You might be able to use > http://codesearch.google.com/#OAMlx_jo-ck/src/base/sync_socket.h&type=cs to > avoid this. Done. http://codereview.chromium.org/8400024/diff/4001/ppapi/tests/test_broker.cc#n... ppapi/tests/test_broker.cc:144: int32_t handle = -1; On 2011/10/27 23:21:24, ddorwin wrote: > = PlatformFileToInt(base::kInvalidPlatformFileValue) Done.
Almost done :) http://codereview.chromium.org/8400024/diff/4/ppapi/tests/test_broker.cc File ppapi/tests/test_broker.cc (right): http://codereview.chromium.org/8400024/diff/4/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:142: ASSERT_TRUE (broker_interface_->IsBrokerTrusted(broker)); Remove space. http://codereview.chromium.org/8400024/diff/4/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:163: TestCompletionCallback cb_1(instance_->pp_instance()); I'd be explicit about the parameter since you refer to it: TestCompletionCallback cb_1(instance_->pp_instance(), false); // Not forced. Note also the explanation of the bool literal. You can use this at 170 to avoid an extra constant too. http://codereview.chromium.org/8400024/diff/4/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:164: ASSERT_EQ(PP_ERROR_BADRESOURCE, broker_interface_->Connect( This is a bit hard to read. Maybe this: ASSERT_EQ(PP_ERROR_BADRESOURCE, broker_interface_->Connect( 0, pp::CompletionCallback(cb_1).pp_completion_callback())); http://codereview.chromium.org/8400024/diff/4/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:175: // Test normal case. It's usually best to do one thing in a test, and if you have to add comments like this, it's an indication you might want to make a separate the test. In this case, you might name the above TestConnectAndGetHandle_Failures and the below TestConnectAndGetHandle[_Success]. Thinking along the same lines (and the strange naming "TestConnectAndGetHandle", it might make sense to separate the GetHandle tests and have three tests: TestConnectFails TestGetHandleFails TestConnectAndPipe http://codereview.chromium.org/8400024/diff/4/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:190: sync_socket, kBrokerIsUnsandboxed, sizeof(kBrokerIsUnsandboxed))); Maybe: ASSERT_TRUE(VerifyMessage(sync_socket, kBrokerIsUnsandboxed, sizeof(kBrokerIsUnsandboxed)));
Done for most comments. I'd like to use TestConnectFailure and TestGetHandleFailure instead of *Fails because I saw other tests use TestCreateFailure. http://codereview.chromium.org/8400024/diff/4/ppapi/tests/test_broker.cc File ppapi/tests/test_broker.cc (right): http://codereview.chromium.org/8400024/diff/4/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:142: ASSERT_TRUE (broker_interface_->IsBrokerTrusted(broker)); On 2011/10/28 17:11:29, ddorwin wrote: > Remove space. Done. http://codereview.chromium.org/8400024/diff/4/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:163: TestCompletionCallback cb_1(instance_->pp_instance()); On 2011/10/28 17:11:29, ddorwin wrote: > I'd be explicit about the parameter since you refer to it: > TestCompletionCallback cb_1(instance_->pp_instance(), false); // Not forced. > Note also the explanation of the bool literal. You can use this at 170 to avoid > an extra constant too. Done. http://codereview.chromium.org/8400024/diff/4/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:164: ASSERT_EQ(PP_ERROR_BADRESOURCE, broker_interface_->Connect( On 2011/10/28 17:11:29, ddorwin wrote: > This is a bit hard to read. Maybe this: > ASSERT_EQ(PP_ERROR_BADRESOURCE, > broker_interface_->Connect( > 0, pp::CompletionCallback(cb_1).pp_completion_callback())); Done. http://codereview.chromium.org/8400024/diff/4/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:175: // Test normal case. On 2011/10/28 17:11:29, ddorwin wrote: > It's usually best to do one thing in a test, and if you have to add comments > like this, it's an indication you might want to make a separate the test. > > In this case, you might name the above TestConnectAndGetHandle_Failures and the > below TestConnectAndGetHandle[_Success]. > > Thinking along the same lines (and the strange naming "TestConnectAndGetHandle", > it might make sense to separate the GetHandle tests and have three tests: > TestConnectFails > TestGetHandleFails > TestConnectAndPipe Done. http://codereview.chromium.org/8400024/diff/4/ppapi/tests/test_broker.cc#newc... ppapi/tests/test_broker.cc:190: sync_socket, kBrokerIsUnsandboxed, sizeof(kBrokerIsUnsandboxed))); On 2011/10/28 17:11:29, ddorwin wrote: > Maybe: > ASSERT_TRUE(VerifyMessage(sync_socket, kBrokerIsUnsandboxed, > sizeof(kBrokerIsUnsandboxed))); Done.
lgtm LGTM with nits. After addressing those and uploading, you can run "git try" to start the trybots. Once those pass, you can ask someone from ppapi/OWNERS for approval. http://codereview.chromium.org/8400024/diff/1005/ppapi/tests/test_broker.cc File ppapi/tests/test_broker.cc (right): http://codereview.chromium.org/8400024/diff/1005/ppapi/tests/test_broker.cc#n... ppapi/tests/test_broker.cc:85: sync_socket.Receive(message_received.get(), length); Should we check that the return value is length? http://codereview.chromium.org/8400024/diff/1005/ppapi/tests/test_broker.cc#n... ppapi/tests/test_broker.cc:177: PP_Resource broker = broker_interface_->CreateTrusted( This _could_ be moved down to 185. http://codereview.chromium.org/8400024/diff/1005/ppapi/tests/test_broker.cc#n... ppapi/tests/test_broker.cc:203: ASSERT_NE(handle, kInvalidHandle); swap. Expected on the left. (That's the GTest convention, and we should follow it even though this isn't really GTest.)
http://codereview.chromium.org/8400024/diff/1005/ppapi/tests/test_broker.cc File ppapi/tests/test_broker.cc (right): http://codereview.chromium.org/8400024/diff/1005/ppapi/tests/test_broker.cc#n... ppapi/tests/test_broker.cc:85: sync_socket.Receive(message_received.get(), length); On 2011/10/28 19:05:59, ddorwin wrote: > Should we check that the return value is length? Done. http://codereview.chromium.org/8400024/diff/1005/ppapi/tests/test_broker.cc#n... ppapi/tests/test_broker.cc:177: PP_Resource broker = broker_interface_->CreateTrusted( On 2011/10/28 19:05:59, ddorwin wrote: > This _could_ be moved down to 185. Done.
If look good to you, could you please help on the try bot again? Thanks!
Hello David, Just uploaded a new patch to remove the dependency on "src/base/..." files which broke the component build. Please help try it out on Win and Mac if it looks good. -xiaohan
Hello David, Disabled TestConnectAndPipe() in Windows. Please review again. -xiaohan
Thanks. LG overall. Mostly nits. http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc File ppapi/tests/test_broker.cc (right): http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:56: const int32_t kInvalidHandle = PlatformFileToInt(kInvalidPlatformFileValue); This would be a bad idea in non-test code. It's actually only used in one function. Also, since PlatformFile is defined in this file, you could just define int32_t values in the constant definitions above and eliminate PlatformFileToInt(). http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:58: bool ReadFromPlatformFile(PlatformFile file, char* buffer, parameter order should be input then outputs. Change the write function too so there is a consistent ordering. http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:59: size_t bytes_to_read, size_t& bytes_read) { output parameters must be pointers per coding style. same below. http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:74: return true; maybe a newline for readability and to separate from the ifdef block. same below. http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:91: return false; indent http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:107: if (!WriteToPlatformFile(file, message, message_len, bytes_written) || This might be easier to read if written positively. And it can be simplified to just "return success && equal;" http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:114: bool ReadMessage(PlatformFile file, char* message, size_t message_len) { parameter ordering. http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:124: size_t length_sent) { length_sent doesn't make sense in the context of this function. Maybe in the callers, but not here. You might reorder these parameters to match the new ReadMessage order too. http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:151: ::unlink(file_name); why unlink instead of remove? Maybe comment. http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:160: ::fclose(file); does it need to be deleted in case part of it was written? There are other cases below where we leave the file too. Oh well, I guess. This would all be easier if we could use EXPECT_*. *sigh* http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:166: Clear file just to be safe. http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:214: } comment that the plugin will handle the unsandboxed failure (and thus no else). http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:316: ASSERT_TRUE(VerifyMessage(file, kBrokerUnsandboxed, Would this hang if the broker was sandboxed and never sent the message? Is there a reasonable way to avoid this?
Mostly done as commented. Thanks! http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc File ppapi/tests/test_broker.cc (right): http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:56: const int32_t kInvalidHandle = PlatformFileToInt(kInvalidPlatformFileValue); On 2011/11/14 18:19:39, ddorwin wrote: > This would be a bad idea in non-test code. It's actually only used in one > function. Also, since PlatformFile is defined in this file, you could just > define int32_t values in the constant definitions above and eliminate > PlatformFileToInt(). Done. http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:58: bool ReadFromPlatformFile(PlatformFile file, char* buffer, On 2011/11/14 18:19:39, ddorwin wrote: > parameter order should be input then outputs. Change the write function too so > there is a consistent ordering. Done. http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:59: size_t bytes_to_read, size_t& bytes_read) { On 2011/11/14 18:19:39, ddorwin wrote: > output parameters must be pointers per coding style. same below. Done. http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:74: return true; On 2011/11/14 18:19:39, ddorwin wrote: > maybe a newline for readability and to separate from the ifdef block. same > below. Done. http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:91: return false; On 2011/11/14 18:19:39, ddorwin wrote: > indent Done. http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:107: if (!WriteToPlatformFile(file, message, message_len, bytes_written) || On 2011/11/14 18:19:39, ddorwin wrote: > This might be easier to read if written positively. And it can be simplified to > just "return success && equal;" I like this :-) http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:107: if (!WriteToPlatformFile(file, message, message_len, bytes_written) || On 2011/11/14 18:19:39, ddorwin wrote: > This might be easier to read if written positively. And it can be simplified to > just "return success && equal;" Done. http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:114: bool ReadMessage(PlatformFile file, char* message, size_t message_len) { On 2011/11/14 18:19:39, ddorwin wrote: > parameter ordering. Done. http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:124: size_t length_sent) { On 2011/11/14 18:19:39, ddorwin wrote: > length_sent doesn't make sense in the context of this function. Maybe in the > callers, but not here. > > You might reorder these parameters to match the new ReadMessage order too. Done. http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:151: ::unlink(file_name); On 2011/11/14 18:19:39, ddorwin wrote: > why unlink instead of remove? Maybe comment. If path does not name a directory, remove(path) shall be equivalent to unlink(path). Will change. http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:151: ::unlink(file_name); On 2011/11/14 18:19:39, ddorwin wrote: > why unlink instead of remove? Maybe comment. Done. http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:160: ::fclose(file); On 2011/11/14 18:19:39, ddorwin wrote: > does it need to be deleted in case part of it was written? There are other cases > below where we leave the file too. Oh well, I guess. > This would all be easier if we could use EXPECT_*. *sigh* Done. http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:166: On 2011/11/14 18:19:39, ddorwin wrote: > Clear file just to be safe. Add comment at the top of the function to warn about possible file leak and that it'll be fixed once these tests are moved to Google Test. http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:214: } On 2011/11/14 18:19:39, ddorwin wrote: > comment that the plugin will handle the unsandboxed failure (and thus no else). Done. http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:316: ASSERT_TRUE(VerifyMessage(file, kBrokerUnsandboxed, On 2011/11/14 18:19:39, ddorwin wrote: > Would this hang if the broker was sandboxed and never sent the message? Is there > a reasonable way to avoid this? Both on POSIX and Win the socket pair/named pipes are created with NON_BLOCKING/PIPE_NOWAIT set. So if the message is not available for read for any reason, VerifyMessage() will return false instead of hang.
lgtm Now you can get OWNERS review and we'll try bot one more time after that (since platform-specific code was changed). http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc File ppapi/tests/test_broker.cc (right): http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:166: On 2011/11/14 22:24:30, xhwang wrote: > On 2011/11/14 18:19:39, ddorwin wrote: > > Clear file just to be safe. > > Add comment at the top of the function to warn about possible file leak and that > it'll be fixed once these tests are moved to Google Test. Sorry, I meant: file = NULL; since we're going to re-assign it below for a read. http://codereview.chromium.org/8400024/diff/34002/ppapi/tests/test_broker.cc File ppapi/tests/test_broker.cc (right): http://codereview.chromium.org/8400024/diff/34002/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:262: // Callback force asynced. Connect will return PP_OK_COMPLETIONPENDING and the "forced async" is probably better here and above. "force_async'd" might be another option, but is weird.
Done. http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc File ppapi/tests/test_broker.cc (right): http://codereview.chromium.org/8400024/diff/29001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:166: On 2011/11/14 22:34:50, ddorwin wrote: > On 2011/11/14 22:24:30, xhwang wrote: > > On 2011/11/14 18:19:39, ddorwin wrote: > > > Clear file just to be safe. > > > > Add comment at the top of the function to warn about possible file leak and > that > > it'll be fixed once these tests are moved to Google Test. > > Sorry, I meant: > file = NULL; > since we're going to re-assign it below for a read. Done. http://codereview.chromium.org/8400024/diff/34002/ppapi/tests/test_broker.cc File ppapi/tests/test_broker.cc (right): http://codereview.chromium.org/8400024/diff/34002/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:262: // Callback force asynced. Connect will return PP_OK_COMPLETIONPENDING and the On 2011/11/14 22:34:50, ddorwin wrote: > "forced async" is probably better here and above. "force_async'd" might be > another option, but is weird. Done.
Hello piman and brettw, Could you please do a OWNERS review on this change? This change adds more tests on the pepper broker. I used to use "base" classes/functions (e.g. base::SyncSocket, base::ScopedTempDir) but later found that it will break component build. So I wrote some part of the test from scratch and resulted in this not so clean code. -xhwang
http://codereview.chromium.org/8400024/diff/37002/ppapi/tests/test_broker.cc File ppapi/tests/test_broker.cc (right): http://codereview.chromium.org/8400024/diff/37002/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:20: #include "Windows.h" Should be written <windows.h> http://codereview.chromium.org/8400024/diff/37002/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:62: ssize_t ret = ::read(file, buffer, bytes_to_read); FYI, ::read and ::write may fail to fully read/write bytes_to_read bytes if interrupted by a signal, and may need to be restarted (see HANDLE_EINTR in base/eintr_wrapper). I don't know if the windows equivalents have this issue too. http://codereview.chromium.org/8400024/diff/37002/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:142: #endif I'd say, at this point, you should know whether you're sandboxed or not. Creating the temp file should be disallowed on all platforms if the sandbox is on. So I think you could stop here rather than adding more complexity (and ifdefs) below. http://codereview.chromium.org/8400024/diff/37002/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:199: if (!WriteMessage(file, sizeof(kBrokerUnsandboxed), kBrokerUnsandboxed)) { It would be better I think to write a response message whether the broker is sandboxed or not. Right now I think you're relying on the fact that you close the socket below to make the ReadMessage on the client side fail. If the connection doesn't properly close for any reason (e.g. recent bug), then the test will hang and timeout, making it flaky and hard to debug. It seems preferable to send a "failure" message that's unambiguous.
Hello piman, Thanks for the great review. I didn't really think about these issues. Modified as commented. For the Windows part I am referring to: http://www.google.com/codesearch#OAMlx_jo-ck/src/base/file_util_win.cc&type=c... -xiaohan http://codereview.chromium.org/8400024/diff/37002/ppapi/tests/test_broker.cc File ppapi/tests/test_broker.cc (right): http://codereview.chromium.org/8400024/diff/37002/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:20: #include "Windows.h" On 2011/11/14 23:49:17, piman wrote: > Should be written <windows.h> Done. http://codereview.chromium.org/8400024/diff/37002/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:62: ssize_t ret = ::read(file, buffer, bytes_to_read); On 2011/11/14 23:49:17, piman wrote: > FYI, ::read and ::write may fail to fully read/write bytes_to_read bytes if > interrupted by a signal, and may need to be restarted (see HANDLE_EINTR in > base/eintr_wrapper). I don't know if the windows equivalents have this issue > too. Done. http://codereview.chromium.org/8400024/diff/37002/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:142: #endif On 2011/11/14 23:49:17, piman wrote: > I'd say, at this point, you should know whether you're sandboxed or not. > Creating the temp file should be disallowed on all platforms if the sandbox is > on. So I think you could stop here rather than adding more complexity (and > ifdefs) below. Done. http://codereview.chromium.org/8400024/diff/37002/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:199: if (!WriteMessage(file, sizeof(kBrokerUnsandboxed), kBrokerUnsandboxed)) { On 2011/11/14 23:49:17, piman wrote: > It would be better I think to write a response message whether the broker is > sandboxed or not. Right now I think you're relying on the fact that you close > the socket below to make the ReadMessage on the client side fail. If the > connection doesn't properly close for any reason (e.g. recent bug), then the > test will hang and timeout, making it flaky and hard to debug. It seems > preferable to send a "failure" message that's unambiguous. Done.
http://codereview.chromium.org/8400024/diff/39001/ppapi/tests/test_broker.cc File ppapi/tests/test_broker.cc (right): http://codereview.chromium.org/8400024/diff/39001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:139: file = ::fdopen(fd, "w"); I'd simplify it even further: fdopen doesn't do anything with the filesystem, just creating a FILE* around the fd. So you could simply remove the file and close the fd, and return success.
Changed as suggested. Thanks! http://codereview.chromium.org/8400024/diff/39001/ppapi/tests/test_broker.cc File ppapi/tests/test_broker.cc (right): http://codereview.chromium.org/8400024/diff/39001/ppapi/tests/test_broker.cc#... ppapi/tests/test_broker.cc:139: file = ::fdopen(fd, "w"); On 2011/11/15 01:58:14, piman wrote: > I'd simplify it even further: fdopen doesn't do anything with the filesystem, > just creating a FILE* around the fd. > So you could simply remove the file and close the fd, and return success. Done. Thanks!
lgtm
Removed dependency on src/base completely. Also use #if defined(_MSC_VER) to distinguish between Windows and Posix builds as we did in test_core.cc.
lgtm
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/8400024/41004
Presubmit check for 8400024-41004 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Missing LGTM from an OWNER for: content/common/sandbox_init_mac.cc,content/common/sandbox_policy.cc Presubmit checks took 2.1s to calculate. Was the presubmit check useful? Please send feedback & hate mail to maruel@chromium.org!
Hello jam, I need your OWNERS review on the following two content/common files: content/common/sandbox_init_mac.cc content/common/sandbox_policy.cc Thanks, xhwang
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/8400024/41004
Change committed as 110241 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
