Collect spelling service feedback on Mac

Collect spelling service feedback on Mac

This CL enables collecting feedback for spelling service on Mac. Mac uses a
different code path for spellchecking, because Mac's built-in spellcheck lives 
in the browser process. Other operating systems keep the built-in spellcheck in
the renderer process. Other operating systems are already collecting the 
feedback for spelling service.

This CL also modifies the IPC message "SpellCheckHostMsg_RequestTextCheck" to
send a list of existing misspellings (SpellCheckMarker) together with the
spellcheck request from the renderer to the browser on Mac. Other operating
systems use the "SpellCheckHostMsg_CallSpellingService" message, which already
passes a list of existing misspellings with the spellcheck request.

BUG=170514
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203227

[please use gerrit instead, 2013-05-24 21:43:24]

Groby: PTAL

Julien: PTAL chrome/common/spellcheck_messages.h

[groby-ooo-7-16, 2013-05-24 23:01:33]

lgtm

[please use gerrit instead, 2013-05-28 22:43:48]

Julien: ping...

[jln (very slow on Chromium), 2013-05-28 23:24:52]

Isn't there a vulnerability in browser/spellchecker/feedback_sender.cc,
FeedbackSender::OnSpellcheckResults() ?

I don't think markers is ever checked, and we do:

  for (size_t i = 0; i < markers.size(); ++i)
    marker_map[markers[i].offset] = markers[i].hash;

Did I miss something?

https://codereview.chromium.org/15861019/diff/8/chrome/browser/spellchecker/s...
File chrome/browser/spellchecker/spellcheck_message_filter_mac.cc (right):

https://codereview.chromium.org/15861019/diff/8/chrome/browser/spellchecker/s...
chrome/browser/spellchecker/spellcheck_message_filter_mac.cc:173: markers_);
If I'm not mistaken, at this stage markers_ is still completely unchecked.

https://codereview.chromium.org/15861019/diff/8/chrome/common/spellcheck_mess...
File chrome/common/spellcheck_messages.h (right):

https://codereview.chromium.org/15861019/diff/8/chrome/common/spellcheck_mess...
chrome/common/spellcheck_messages.h:28: IPC_STRUCT_TRAITS_MEMBER(offset)
Offset here seems dangerous!

[jln (very slow on Chromium), 2013-05-28 23:30:46]

On 2013/05/28 23:24:52, Julien Tinnes wrote:

> Did I miss something?

comex@ pointed out to me that I'm an idiot and marker_map is (surprisingly!) a
map.. (Also it was defined one line above).

[please use gerrit instead, 2013-05-29 00:16:21]

Julien: PTAL. I've modified the CL to make sure that offset is always valid.
(Using CHECKs instead of DCHECKs per
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACH...)

[groby-ooo-7-16, 2013-05-29 00:27:19]

From the link you just sent:
"Because this takes down the whole browser, sometimes there are better options
than CHECK().  For example, if a renderer sends the browser process a malformed
IPC, an attacker may control the renderer, but we can simply kill the offending
renderer instead of crashing the whole browser."

At the very least, we should kill the renderer instead of crashing the browser.

And unless we use offset to actually index into the text, we can simply _ignore_
bad markers. As is, this seems to be a bit of a sledgehammer to me. (Deferring
to security peeps on final judgment, though)

[jln (very slow on Chromium), 2013-05-29 00:28:44]

On 2013/05/29 00:16:21, Rouslan Solomakhin wrote:

In general, we don't want to allow renderers to trivially DoS the browser
process. Would it be possible to ignore invalid messages instead?

[please use gerrit instead, 2013-05-29 00:37:43]

On 2013/05/29 00:28:44, Julien Tinnes wrote:
> On 2013/05/29 00:16:21, Rouslan Solomakhin wrote:
> 
> In general, we don't want to allow renderers to trivially DoS the browser
> process. Would it be possible to ignore invalid messages instead?

Yes, I will do that.

[please use gerrit instead, 2013-05-29 17:07:19]

Julien: PTAL. I'm filtering out the markers with invalid offsets.

[jln (very slow on Chromium), 2013-05-29 23:56:26]

chrome/common/spellcheck_messages.h lgtm

[jln (very slow on Chromium), 2013-05-29 23:56:40]

(not mandatory comments)

https://chromiumcodereview.appspot.com/15861019/diff/57001/chrome/browser/spe...
File chrome/browser/spellchecker/spellcheck_message_filter.cc (right):

https://chromiumcodereview.appspot.com/15861019/diff/57001/chrome/browser/spe...
chrome/browser/spellchecker/spellcheck_message_filter.cc:120: markers.erase(
Do you want to add a comment here ? Will also make it easier for security
reviewers.

https://chromiumcodereview.appspot.com/15861019/diff/57001/chrome/browser/spe...
File chrome/browser/spellchecker/spellcheck_message_filter_mac.cc (right):

https://chromiumcodereview.appspot.com/15861019/diff/57001/chrome/browser/spe...
chrome/browser/spellchecker/spellcheck_message_filter_mac.cc:280: markers.erase(
Comment?

[please use gerrit instead, 2013-05-30 00:14:13]

https://chromiumcodereview.appspot.com/15861019/diff/57001/chrome/browser/spe...
File chrome/browser/spellchecker/spellcheck_message_filter.cc (right):

https://chromiumcodereview.appspot.com/15861019/diff/57001/chrome/browser/spe...
chrome/browser/spellchecker/spellcheck_message_filter.cc:120: markers.erase(
On 2013/05/29 23:56:40, Julien Tinnes wrote:
> Do you want to add a comment here ? Will also make it easier for security
> reviewers.

Added a comment:

// Erase invalid markers (with offsets out of boundaries of text length).

https://chromiumcodereview.appspot.com/15861019/diff/57001/chrome/browser/spe...
File chrome/browser/spellchecker/spellcheck_message_filter_mac.cc (right):

https://chromiumcodereview.appspot.com/15861019/diff/57001/chrome/browser/spe...
chrome/browser/spellchecker/spellcheck_message_filter_mac.cc:280: markers.erase(
On 2013/05/29 23:56:40, Julien Tinnes wrote:
> Comment?

Ditto.

[commit-bot: I haz the power, 2013-05-30 00:18:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/15861019/84001

[commit-bot: I haz the power, 2013-05-30 01:05:58]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-30 17:11:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/15861019/12003

[commit-bot: I haz the power, 2013-05-30 18:10:33]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[please use gerrit instead, 2013-05-30 19:06:31]

On 2013/05/30 18:10:33, I haz the power (commit-bot) wrote:
> Retried try job too often on mac_rel for step(s) browser_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

Browser tests seem to pass on my Mac. Going to try again.

[commit-bot: I haz the power, 2013-05-30 19:06:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/15861019/12003

[commit-bot: I haz the power, 2013-05-30 21:37:06]

Change committed as 203227