Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
250713003:
Test sending corrupt mojo messages back from javascript. (Closed)
|
Created:
6 years, 8 months ago by Tom Sepez Modified:
6 years, 7 months ago CC:
chromium-reviews, viettrungluu+watch_chromium.org, jam, Aaron Boodman, darin-cc_chromium.org, darin (slow to review), ben+mojo_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionTest sending corrupt mojo messages back from javascript.
This CL adds hooks to allow an outgoing message to be re-written just
before it is handed off to C++ for transmission. I then build a simple
bit-flipping test based upon this to see how the C++ side stands up to
this (under ASAN).
Another small change is to pass the number of iterations from the C++ side so
that there no longer is a need to keep two constants in sync with each other.
Another small change is to pull some of repeated code dealing with isolated
testing into its own function to avoid duplication.
The test is currently disabled pending the associated bug. This does not
fix the bug but introduces a realiable repro for it.
BUG=366797
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267925
Patch Set 1 #Patch Set 2 : Fix Uint32 => Uint8 for consistency. #
Total comments: 4
Patch Set 3 : Don't modify the production JS. #Patch Set 4 : Tidy code, avoid need for intermediate objects. #Patch Set 5 : Fuzzing from apps/test unit tests. #Patch Set 6 : Consolidate some duplicated sections. #Patch Set 7 : Missing TestFinished() method in bit flip test, and add comment. #Patch Set 8 : Rebase. #
Messages
Total messages: 28 (0 generated)
Sky, MP, Darin, please take a look. Thanks.
https://codereview.chromium.org/250713003/diff/20001/mojo/public/js/bindings/... File mojo/public/js/bindings/connection.js (right): https://codereview.chromium.org/250713003/diff/20001/mojo/public/js/bindings/... mojo/public/js/bindings/connection.js:25: } We shouldn't need this test code in the production JavaScript....
https://codereview.chromium.org/250713003/diff/20001/mojo/public/js/bindings/... File mojo/public/js/bindings/connection.js (right): https://codereview.chromium.org/250713003/diff/20001/mojo/public/js/bindings/... mojo/public/js/bindings/connection.js:25: } On 2014/04/25 00:27:54, abarth wrote: > We shouldn't need this test code in the production JavaScript.... As Adam pointed out to me when I tried this, you can mock out the 'core' functions to avoid this. You'll want to override core.writeMessage. See https://code.google.com/p/chromium/codesearch#chromium/src/mojo/apps/js/bindi... for an example.
https://codereview.chromium.org/250713003/diff/20001/content/test/data/web_ui... File content/test/data/web_ui_mojo.js (right): https://codereview.chromium.org/250713003/diff/20001/content/test/data/web_ui... content/test/data/web_ui_mojo.js:67: })()); Rather than hooking in like this, you should be able to hook into the JavaScript objects directly without cooperation from the production code.
Meta question posed to Matt in other thread: How come tests exercising JS bindings code is in content? The WebUIMojoBrowserTest is intended to be an end to end test that wires up the necessary webui pieces to enable using mojo. Shouldn't tests that are exercising lower level JS bindings encoding be in mojo?
On 2014/04/25 15:27:05, sky wrote: > Meta question posed to Matt in other thread: How come > tests exercising JS bindings code is in content? The WebUIMojoBrowserTest is > intended to be an end to end test that wires up the necessary webui pieces to > enable using mojo. Shouldn't tests that are exercising lower level JS bindings > encoding be in mojo? This isn't really a JS bindings test, rather its a test of both JS and C side using JS as a tool to accomplish it. Using JS is important to get simple repro's, and to allow for generation of a variety of test cases down the road in ClusterFuzz. I'm open to suggestions about where to put it.
https://codereview.chromium.org/250713003/diff/20001/mojo/public/js/bindings/... File mojo/public/js/bindings/connection.js (right): https://codereview.chromium.org/250713003/diff/20001/mojo/public/js/bindings/... mojo/public/js/bindings/connection.js:25: } On 2014/04/25 00:27:54, abarth wrote: > We shouldn't need this test code in the production JavaScript.... Yep. Was just trying to preserve some sort of layering, getting the object from the top-level JS and ripping out its guts is always possible so long as the names of the internal fields don't change. Yay JS. (In other words, Will do). I'm not sure mocking is appropriate here, we need to have the message flow all the way to the C++ side and then to the browser, etc. Maybe I'm missing something?
On 2014/04/25 18:30:33, Tom Sepez wrote: > https://codereview.chromium.org/250713003/diff/20001/mojo/public/js/bindings/... > File mojo/public/js/bindings/connection.js (right): > > https://codereview.chromium.org/250713003/diff/20001/mojo/public/js/bindings/... > mojo/public/js/bindings/connection.js:25: } > On 2014/04/25 00:27:54, abarth wrote: > > We shouldn't need this test code in the production JavaScript.... > Yep. Was just trying to preserve some sort of layering, getting the object from > the top-level JS and ripping out its guts is always possible so long as the > names of the internal fields don't change. Yay JS. (In other words, Will do). *THIS IS JAVASCRIPT* > I'm not sure mocking is appropriate here, we need to have the message flow all > the way to the C++ side and then to the browser, etc. Maybe I'm missing > something? I think I was using the term mocking incorrectly. I just meant that you can interpose yourself wherever you like by hacking up the object graph. If you're hacking into the |core| module, I won't worry too much about those functions changing. They're supposed to be binary stable on the C++ side.
On Fri, Apr 25, 2014 at 10:52 AM, <tsepez@chromium.org> wrote: > On 2014/04/25 15:27:05, sky wrote: >> >> Meta question posed to Matt in other thread: How come >> tests exercising JS bindings code is in content? The WebUIMojoBrowserTest >> is >> intended to be an end to end test that wires up the necessary webui pieces >> to >> enable using mojo. Shouldn't tests that are exercising lower level JS >> bindings >> encoding be in mojo? > > This isn't really a JS bindings test, rather its a test of both JS and C > side > using JS as a tool to accomplish it. Using JS is important to get simple > repro's, and to allow for generation of a variety of test cases down the > road in > ClusterFuzz. I'm open to suggestions about where to put it. I think it's better for this code to live in mojo land. Maybe in mojo_js_unittests. -Scott > > https://codereview.chromium.org/250713003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> I think it's better for this code to live in mojo land. Maybe in > mojo_js_unittests. > Hey Scott, does the mojo_js_unittests "framework" actually result in a situation where messages are transported inter-process and decoded in C++? That's the code path that needs to be covered by this fuzzing.
On 2014/04/25 19:38:23, Tom Sepez wrote: > > I think it's better for this code to live in mojo land. Maybe in > > mojo_js_unittests. > > > Hey Scott, does the mojo_js_unittests "framework" actually result in a situation > where messages are transported inter-process and decoded in C++? That's the > code path that needs to be covered by this fuzzing. Does it need to be transported cross-process, or would it suffice to go through a MessagePipe in-process? I agree with Scott that it makes sense to put these tests somewhere in mojo/. We'd need a new test type that sends messages from C++ to JS, though.
> Does it need to be transported cross-process, or would it suffice to go through > a MessagePipe in-process? I agree with Scott that it makes sense to put these > tests somewhere in mojo/. We'd need a new test type that sends messages from C++ > to JS, though. I'd expect message pipe in the same process is good enough.
On Fri, Apr 25, 2014 at 12:38 PM, <tsepez@chromium.org> wrote: > > I think it's better for this code to live in mojo land. Maybe in >> >> mojo_js_unittests. > > > Hey Scott, does the mojo_js_unittests "framework" actually result in a > situation > where messages are transported inter-process and decoded in C++? That's the > code path that needs to be covered by this fuzzing. You will have to set it up. -Scott > > https://codereview.chromium.org/250713003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> Hey Scott, does the mojo_js_unittests "framework" actually result in a > > situation > > where messages are transported inter-process and decoded in C++? That's the > > code path that needs to be covered by this fuzzing. > > You will have to set it up. > > -Scott So I spent some time sniffing around the code base, and it wasn't apparent to me how to set this up so that I have c++ and JS execution at the same time. Maybe this moves to mojo_browsertests?
This should all be possible from a unit test. I'll see if I can find some time today to right an example of what I'm suggesting. -Scott On Mon, Apr 28, 2014 at 10:29 AM, <tsepez@chromium.org> wrote: > > Hey Scott, does the mojo_js_unittests "framework" actually result in a >> >> > situation >> > where messages are transported inter-process and decoded in C++? That's >> > the >> > code path that needs to be covered by this fuzzing. > > >> You will have to set it up. > > >> -Scott > > So I spent some time sniffing around the code base, and it wasn't apparent > to me > how to set this up so that I have c++ and JS execution at the same time. > Maybe > this moves to mojo_browsertests? > > https://codereview.chromium.org/250713003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sky, please re-review. This requires https://codereview.chromium.org/268593002/
LGTM
The CQ bit was checked by tsepez@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tsepez@chromium.org/250713003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium
The CQ bit was checked by tsepez@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tsepez@chromium.org/250713003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel on tryserver.chromium
The CQ bit was checked by tsepez@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tsepez@chromium.org/250713003/140001
Message was sent while issue was closed.
Change committed as 267925 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
