Fixed crash in WebFrameImpl::resetMatchCount function.

Fixed crash in WebFrameImpl::resetMatchCount function.
Now function ensureTextFinder is used to create TextFinder object if it doesn't exists.

BUG=359983
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175504

[Andrey Kraynov, 2014-04-10 14:53:45]

yosin@, would you review this please?

As I see from the crash stack, there can be a situation when function
WebFrameImpl::resetMatchCount is called but m_textFinder still uninitialized. So
I use ensureTextFinder() to create m_textFinder object.

But I'm not able to reproduce the problem locally.
I think that it should be a multi-frame document with focused element.
I don't have access to the detailed report or minimal testcase.
Could you send me that testcase please?
I think it would be great to add a unit-test for that case.

[yosin_UTC9, 2014-04-11 01:45:11]

ACK

This is what I would like to have. ;-)
Please make generated test case readable and put into this patch.

Thanks!

[yosin_UTC9, 2014-04-30 01:54:38]

Ping...

[Andrey Kraynov, 2014-05-14 11:47:54]

On 2014/04/30 01:54:38, Yosi OOF til May 12 wrote:
> Ping...

Sorry for delay =(
 
I think that this test should not be a LayoutTest, because function
WebLocalFrameImpl::resetMatchCount cannot be called from LayoutTests.
Correct me, if I wrong, please.
That's why I decided to create a unit-test in WebFrameTest.cpp.
This new test loads HTML file with refactored script from crash description.
After that, it tries to find some string and calls mainFrame->resetMatchCount().
 
What do you think about that?
Is it OK?
 
I rebased my CL to fresh master and added test.

[yosin_UTC9, 2014-05-19 05:26:22]

ACK

Sorry for later response. I was OOO last week.

+tkent@ for OWNERS review.

https://codereview.chromium.org/229043004/diff/60001/Source/web/tests/WebFram...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/229043004/diff/60001/Source/web/tests/WebFram...
Source/web/tests/WebFrameTest.cpp:3460: for (WebFrame* frame = mainFrame; frame;
frame = frame->traverseNext(false))
nit: Please add "{}" to follow Blink coding standard.

https://codereview.chromium.org/229043004/diff/60001/Source/web/tests/data/fi...
File Source/web/tests/data/find_in_generated_frame.html (right):

https://codereview.chromium.org/229043004/diff/60001/Source/web/tests/data/fi...
Source/web/tests/data/find_in_generated_frame.html:8: " svgElem1 =
document.createElementNS('http://www.w3.org/2000/svg', 'svg');",
super-nit: It is better to use function closure rather thane eval;
 - for coloring in editor
 - syntax check during loading


var expression = [
  function() { svgElem1 = ... },
  function() { ... },
  ...
];

[tkent, 2014-05-19 05:32:23]

lgtm

[Andrey Kraynov, 2014-05-20 21:34:46]

Thank you!
I rebased CL and fixed remarks.

https://codereview.chromium.org/229043004/diff/60001/Source/web/tests/WebFram...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/229043004/diff/60001/Source/web/tests/WebFram...
Source/web/tests/WebFrameTest.cpp:3460: for (WebFrame* frame = mainFrame; frame;
frame = frame->traverseNext(false))
On 2014/05/19 05:26:22, yosi wrote:
> nit: Please add "{}" to follow Blink coding standard.

Done.

https://codereview.chromium.org/229043004/diff/60001/Source/web/tests/data/fi...
File Source/web/tests/data/find_in_generated_frame.html (right):

https://codereview.chromium.org/229043004/diff/60001/Source/web/tests/data/fi...
Source/web/tests/data/find_in_generated_frame.html:8: " svgElem1 =
document.createElementNS('http://www.w3.org/2000/svg', 'svg');",
On 2014/05/19 05:26:22, yosi wrote:
> super-nit: It is better to use function closure rather thane eval;

I tried to stick to the original, but I agree with you.
Done.

[Andrey Kraynov, 2014-05-23 15:46:21]

The CQ bit was checked by iceman@yandex-team.ru

[commit-bot: I haz the power, 2014-05-23 15:46:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/iceman@yandex-team.ru/229043004/100001

[commit-bot: I haz the power, 2014-05-23 17:03:55]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/9356)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/8807)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/8925)

[commit-bot: I haz the power, 2014-05-23 17:21:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-23 17:21:09]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/9366)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/8817)

[yosin_UTC9, 2014-05-29 01:33:21]

We can minimize test case more, but it is optional.
I hope using "onload" solve flakyness.

https://codereview.chromium.org/229043004/diff/140001/Source/web/tests/data/f...
File Source/web/tests/data/find_in_generated_frame.html (right):

https://codereview.chromium.org/229043004/diff/140001/Source/web/tests/data/f...
Source/web/tests/data/find_in_generated_frame.html:8: function () {
You can move svgElem1, svgElem2, titleElem initialization into the last
function.

https://codereview.chromium.org/229043004/diff/140001/Source/web/tests/data/f...
Source/web/tests/data/find_in_generated_frame.html:12: svgElem2 =
document.createElementNS('http://www.w3.org/2000/svg', 'svg');
nit: we can remove this. |svgElem2| never used.

https://codereview.chromium.org/229043004/diff/140001/Source/web/tests/data/f...
Source/web/tests/data/find_in_generated_frame.html:15: titleElem =
document.createElementNS('http://www.w3.org/2000/svg', 'title');
nit: we can remove this. |titleElem|. never used.

https://codereview.chromium.org/229043004/diff/140001/Source/web/tests/data/f...
Source/web/tests/data/find_in_generated_frame.html:19: if (!svgElem1.parentNode)
{
nit: svgElem1.parentNode is null here. Because it is created in L9.

https://codereview.chromium.org/229043004/diff/140001/Source/web/tests/data/f...
Source/web/tests/data/find_in_generated_frame.html:22:
svgElem1.parentNode.insertBefore(generatedFrame, svgElem1);
It seems you can build |generatedFrame| by using "srcdoc" attribute, e.g.
data:text/html,<iframe srcdoc="<b>Foo</b>" width="200" height="300"></frame>

I guess we don't need to use SVG element.

https://codereview.chromium.org/229043004/diff/140001/Source/web/tests/data/f...
Source/web/tests/data/find_in_generated_frame.html:33: setTimeout(runTasks, 0);
nit: window.onload = runTasks;

I think this can resolve timing issue.

timeout can be fired before BODY element rendered; parser has one shot timer to
start rendering slow channel. BOT is always slow.

[Andrey Kraynov, 2014-06-03 20:59:09]

On 2014/05/29 01:33:21, yosi wrote:
> We can minimize test case more, but it is optional.
> I hope using "onload" solve flakyness.
> It seems you can build |generatedFrame| by using "srcdoc" attribute, e.g.
> data:text/html,<iframe srcdoc="<b>Foo</b>" width="200" height="300"></frame>
> 
> I guess we don't need to use SVG element.
> 
>
https://codereview.chromium.org/229043004/diff/140001/Source/web/tests/data/f...
> Source/web/tests/data/find_in_generated_frame.html:33: setTimeout(runTasks,
0);
> nit: window.onload = runTasks;
> 
> I think this can resolve timing issue.
> 
> timeout can be fired before BODY element rendered; parser has one shot timer
to
> start rendering slow channel. BOT is always slow.

I think that you are right, and the test case could be minimized.
I removed unnecessary JS code and used window.onload event.
Now test can pass successfully (I tested Release build locally).
Thank you for the advise!

[yosin_UTC9, 2014-06-04 01:06:02]

https://codereview.chromium.org/229043004/diff/190001/Source/web/tests/data/f...
File Source/web/tests/data/find_in_generated_frame.html (right):

https://codereview.chromium.org/229043004/diff/190001/Source/web/tests/data/f...
Source/web/tests/data/find_in_generated_frame.html:1: <!DOCTYPE html>
Cool!

[Andrey Kraynov, 2014-06-04 16:44:21]

The CQ bit was checked by iceman@yandex-team.ru

[commit-bot: I haz the power, 2014-06-04 16:45:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/iceman@yandex-team.ru/229043004/190001

[commit-bot: I haz the power, 2014-06-04 18:20:45]

Change committed as 175504