Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2335483004:
Limit depth of parsing of dictionaries returned by JS evaluation. (Closed)
|
Created:
4 years, 3 months ago by jif Modified:
4 years, 3 months ago Reviewers:
Eugene But (OOO till 7-30) CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLimit depth of parsing of dictionaries returned by JS evaluation.
BUG=645486
Committed: https://crrev.com/8c6bd84b9151d53c938205ad7809cd7784980403
Cr-Commit-Position: refs/heads/master@{#418289}
Patch Set 1 #
Total comments: 23
Patch Set 2 : Addressed comments. #
Total comments: 2
Patch Set 3 : Addressed comment. #
Messages
Total messages: 22 (14 generated)
Description was changed from ========== Limit depth of parsing. BUG=645486 ========== to ========== Limit depth of parsing. BUG=645486 ==========
jif@chromium.org changed reviewers: + eugenebut@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Limit depth of parsing. BUG=645486 ========== to ========== Limit depth of parsing of dictionaries returned by JS evaluation. BUG=645486 ==========
ptal
Thanks! lgtm https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... File ios/web/web_state/ui/web_view_js_utils.mm (right): https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils.mm:22: std::unique_ptr<base::Value> ValueResultFromWKResult(id wk_result, Could you please move this function to anonymous namespace and add comments. https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils.mm:29: if (max_depth < 0) { NIT: Should this be |max_depth <= 0| ? Otherwise recursion depth can be 7 https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils.mm:52: NSString* objCString = base::mac::ObjCCast<NSString>(key); s/objCString/obj_c_string https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils.mm:53: if (objCString) { Do you need this check? base::SysNSStringToUTF8 handles nil inputs just fine returning empty strings. https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... File ios/web/web_state/ui/web_view_js_utils_unittest.mm (right): https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils_unittest.mm:73: NSDictionary* testDictionary = Since you touching this file, could you please s/testDictionary/test_dictionary https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils_unittest.mm:87: base::DictionaryValue const* innerDictionary = nullptr; And s/innerDictionary/inner_dictionary https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils_unittest.mm:98: // Creates a dictionary with a cycle. NIT: s/Creates/Create We use imperative form for code comments and descriptive for API comments. https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils_unittest.mm:99: NSMutableDictionary* testDictionary = s/testDictionary/test_dictionary Also obj_c_key, current_dictionary, inner_dictionary, current_depth https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils_unittest.mm:106: [testDictionary setObject:testDictionary2 forKey:objCKey]; Do you want to use subscript?: test_dictionary[obj_c_key] = test_dictionary2; https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils_unittest.mm:124: while (currentDepth <= kMaximumParsingRecursionDepth) { Should this be a for loop?: for (int current_depth = 0; current_depth < kMaximumParsingRecursionDepth; current_depth) https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils_unittest.mm:124: while (currentDepth <= kMaximumParsingRecursionDepth) { How about using very large number instead of kMaximumParsingRecursionDepth? If someone change kMaximumParsingRecursionDepth to 10000 this test should ideally catch the stack overflow.
Patchset #2 (id:60001) has been deleted
Thanks. TAL if you want, I'll submit it on Wednesday if you don't post any comments. https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... File ios/web/web_state/ui/web_view_js_utils.mm (right): https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils.mm:22: std::unique_ptr<base::Value> ValueResultFromWKResult(id wk_result, On 2016/09/12 16:12:00, Eugene But wrote: > Could you please move this function to anonymous namespace and add comments. Done. https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils.mm:29: if (max_depth < 0) { On 2016/09/12 16:12:00, Eugene But wrote: > NIT: Should this be |max_depth <= 0| ? > > Otherwise recursion depth can be 7 I briefly looked at the definition of "depth", and it looks like the root node of a tree has a depth of 0. This means that if |max_depth| is 0, |ValueResultFromWKResult| should at least parse the root node. This would not be the case with "max_depth <= 0", and is why is chose "max_depth < 0". Does that make sense? https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils.mm:52: NSString* objCString = base::mac::ObjCCast<NSString>(key); On 2016/09/12 16:12:00, Eugene But wrote: > s/objCString/obj_c_string Done. https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils.mm:53: if (objCString) { On 2016/09/12 16:12:00, Eugene But wrote: > Do you need this check? base::SysNSStringToUTF8 handles nil inputs just fine > returning empty strings. Ah, thanks! Done. https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... File ios/web/web_state/ui/web_view_js_utils_unittest.mm (right): https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils_unittest.mm:73: NSDictionary* testDictionary = On 2016/09/12 16:12:00, Eugene But wrote: > Since you touching this file, could you please s/testDictionary/test_dictionary Done. https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils_unittest.mm:87: base::DictionaryValue const* innerDictionary = nullptr; On 2016/09/12 16:12:00, Eugene But wrote: > And s/innerDictionary/inner_dictionary Done. https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils_unittest.mm:98: // Creates a dictionary with a cycle. On 2016/09/12 16:12:00, Eugene But wrote: > NIT: s/Creates/Create > > We use imperative form for code comments and descriptive for API comments. Done. Thanks for the reminder :) https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils_unittest.mm:99: NSMutableDictionary* testDictionary = On 2016/09/12 16:12:00, Eugene But wrote: > s/testDictionary/test_dictionary > > Also obj_c_key, current_dictionary, inner_dictionary, current_depth Done. https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils_unittest.mm:106: [testDictionary setObject:testDictionary2 forKey:objCKey]; On 2016/09/12 16:12:00, Eugene But wrote: > Do you want to use subscript?: > > test_dictionary[obj_c_key] = test_dictionary2; Done. https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils_unittest.mm:124: while (currentDepth <= kMaximumParsingRecursionDepth) { On 2016/09/12 16:12:00, Eugene But wrote: > Should this be a for loop?: > for (int current_depth = 0; current_depth < kMaximumParsingRecursionDepth; > current_depth) Done. https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils_unittest.mm:124: while (currentDepth <= kMaximumParsingRecursionDepth) { On 2016/09/12 16:12:00, Eugene But wrote: > How about using very large number instead of kMaximumParsingRecursionDepth? If > someone change kMaximumParsingRecursionDepth to 10000 this test should ideally > catch the stack overflow. If someone changes kMaximumParsingRecursionDepth to 10000, then the stack overflow will occur when calling ValueResultFromWKResult in line 115-116.
The CQ bit was checked by jif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Still lgtm https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... File ios/web/web_state/ui/web_view_js_utils.mm (right): https://codereview.chromium.org/2335483004/diff/40001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils.mm:29: if (max_depth < 0) { On 2016/09/13 09:41:09, jif wrote: > On 2016/09/12 16:12:00, Eugene But wrote: > > NIT: Should this be |max_depth <= 0| ? > > > > Otherwise recursion depth can be 7 > > I briefly looked at the definition of "depth", and it looks like the root node > of a tree has a depth of 0. > > This means that if |max_depth| is 0, |ValueResultFromWKResult| should at least > parse the root node. This would not be the case with "max_depth <= 0", and is > why is chose "max_depth < 0". > > Does that make sense? Sure. https://codereview.chromium.org/2335483004/diff/80001/ios/web/web_state/ui/we... File ios/web/web_state/ui/web_view_js_utils.mm (right): https://codereview.chromium.org/2335483004/diff/80001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils.mm:66: } // anonymous namespace s/anonymous namespace/namespace
The CQ bit was checked by jif@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2335483004/#ps100001 (title: "Addressed comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2335483004/diff/80001/ios/web/web_state/ui/we... File ios/web/web_state/ui/web_view_js_utils.mm (right): https://codereview.chromium.org/2335483004/diff/80001/ios/web/web_state/ui/we... ios/web/web_state/ui/web_view_js_utils.mm:66: } // anonymous namespace On 2016/09/13 15:52:55, Eugene But wrote: > s/anonymous namespace/namespace Done.
Message was sent while issue was closed.
Description was changed from ========== Limit depth of parsing of dictionaries returned by JS evaluation. BUG=645486 ========== to ========== Limit depth of parsing of dictionaries returned by JS evaluation. BUG=645486 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Limit depth of parsing of dictionaries returned by JS evaluation. BUG=645486 ========== to ========== Limit depth of parsing of dictionaries returned by JS evaluation. BUG=645486 Committed: https://crrev.com/8c6bd84b9151d53c938205ad7809cd7784980403 Cr-Commit-Position: refs/heads/master@{#418289} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8c6bd84b9151d53c938205ad7809cd7784980403 Cr-Commit-Position: refs/heads/master@{#418289} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
