Fix crash when calling chrome.tabs.move({index:-1}).

Fix crash when calling chrome.tabs.move({index:-1}).

BUG=53990
TEST=ExtensionApiTest.TabMove


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=100343

[mihaip_google.com, 2011-09-08 20:00:09]

I thought you could specify min/max values for parameters in
extension_api.json.

Mihai

On Thu, Sep 8, 2011 at 12:17 PM, <jstritar@chromium.org> wrote:

> Reviewers: Yoyo Zhou,
>
> Description:
> Fix crash when calling chrome.tabs.move({index:-1}).
>
> BUG=53990
> TEST=ExtensionApiTest.TabMove
>
>
> Please review this at
http://codereview.chromium.**org/7787004/<http://codereview.chromium.org/7787...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M chrome/browser/extensions/**extension_tabs_module.cc
>  M chrome/test/data/extensions/**api_test/tabs/basics/move.html
>
>
> Index: chrome/browser/extensions/**extension_tabs_module.cc
> diff --git a/chrome/browser/extensions/**extension_tabs_module.cc
> b/chrome/browser/extensions/**extension_tabs_module.cc
> index dbbfb34a2fc115c24ac7d796952520**5d67d061ea..**
> c8c7fabb976ab05d7186d7c8c519cb**66d491c33e 100644
> --- a/chrome/browser/extensions/**extension_tabs_module.cc
> +++ b/chrome/browser/extensions/**extension_tabs_module.cc
> @@ -1033,7 +1033,10 @@ bool MoveTabFunction::RunImpl() {
>   int new_index;
>   EXTENSION_FUNCTION_VALIDATE(**update_props->GetInteger(
>       keys::kIndexKey, &new_index));
> -  EXTENSION_FUNCTION_VALIDATE(**new_index >= 0);
> +
> +  // Clamp move location to the first position (the upper bound is handled
> +  // later).
> +  new_index = std::max(0, new_index);
>
>   Browser* source_browser = NULL;
>   TabStripModel* source_tab_strip = NULL;
> Index: chrome/test/data/extensions/**api_test/tabs/basics/move.html
> diff --git a/chrome/test/data/extensions/**api_test/tabs/basics/move.html
> b/chrome/test/data/extensions/**api_test/tabs/basics/move.html
> index 9df94f4ae7e74c8b6192e24fac1ea0**7835ed8bbb..**
> a4e8cc581859a70e40c0ca62f2ca8a**e6b3ca6007 100644
> --- a/chrome/test/data/extensions/**api_test/tabs/basics/move.html
> +++ b/chrome/test/data/extensions/**api_test/tabs/basics/move.html
> @@ -80,6 +80,16 @@ chrome.test.runTests([
>         assertEq("chrome://newtab/", tabs[1].url);
>       }));
>     }));
> +  },
> +
> +  // Make sure we don't crash when the move index is out of range.
> +  function moveToInvalidTab() {
> +    chrome.tabs.move(moveTabIds['**b'], {index: -1}, pass(function(tabB)
> {
> +      assertEq(0, tabB.index);
> +      chrome.tabs.move(moveTabIds['**b'], {index: 10000},
> pass(function(tabB) {
> +        assertEq(1, tabB.index);
> +      }));
> +    }));
>   }
>  ]);
>  </script>
>
>
>

[jstritar, 2011-09-08 20:14:11]

Good point- you can, and this already method actually has a minimum specified.
When I backed out my change, the minimum isn't enforced and the chrome.tabs.move
callback has tab.index == -1. I quickly grepped the code and I don't think we
actually do anything with these! I assume we should just throw an error from the
renderer, like we do for invalid types?

On 2011/09/08 20:00:09, mihaip wrote:
> I thought you could specify min/max values for parameters in
> extension_api.json.
> 
> Mihai
>

[jstritar, 2011-09-08 20:16:30]

On 2011/09/08 20:14:11, jstritar wrote:
> Good point- you can, and this already method actually has a minimum specified.
> When I backed out my change, the minimum isn't enforced and the
chrome.tabs.move
> callback has tab.index == -1. I quickly grepped the code and I don't think we
> actually do anything with these! I assume we should just throw an error from
the
> renderer, like we do for invalid types?
> 
> On 2011/09/08 20:00:09, mihaip wrote:
> > I thought you could specify min/max values for parameters in
> > extension_api.json.
> > 
> > Mihai
> >

Actually, there are some checks in JSONSchemaValidator... looking into why
they're not being thrown.

[jstritar, 2011-09-08 20:47:15]

Okay, I fixed the validation issue. The number min/max validation was being
skipped if the min/max was 0.

[Mihai Parparita -not on Chrome, 2011-09-08 20:49:59]

LGTM

http://codereview.chromium.org/7787004/diff/2002/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/tabs/basics/tabs_util.js (right):

http://codereview.chromium.org/7787004/diff/2002/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/tabs/basics/tabs_util.js:5: var fail =
chrome.test.callbackFail;
Nit: I don't think this change is necessary.

[Yoyo Zhou, 2011-09-08 20:52:32]

LGTM

[jstritar, 2011-09-08 20:54:02]

http://codereview.chromium.org/7787004/diff/2002/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/tabs/basics/tabs_util.js (right):

http://codereview.chromium.org/7787004/diff/2002/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/tabs/basics/tabs_util.js:5: var fail =
chrome.test.callbackFail;
On 2011/09/08 20:49:59, Mihai Parparita wrote:
> Nit: I don't think this change is necessary.

Oops, forgot to get rid of that.

[commit-bot: I haz the power, 2011-09-09 04:30:59]

Change committed as 100343