Bug 97807 – [Chromium] Fix the find-in-page implementation for detaching frames.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Leandro Graciá Gil]

Our current find-in-page implementation is not properly tested against detaching frames, as bug 97688 proved to be the case. We should introduce proper test coverage, fix any crashes and ensure that a final reply is always sent.

[Leandro Graciá Gil]

Created attachment 166039 [details]
Patch

[Leandro Graciá Gil]

(In reply to comment #1)
> Created an attachment (id=166039) [details]
> Patch

Tested against Chromium's Find* browser_tests.

[Adam Barth]

Comment on attachment 166039 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166039&action=review

This is great.  A few minor comments.  The main issue is that the unit tests should go through the API rather than manually poking at WebCore internals and potentially putting WebCore into an inconsistent state.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:581
> +    virtual void willDetachPage()

willDetachPage should always occur before frameDestroyed, so m_webFrameImpl will always be non-0 here.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1658
> +    // Detached frame case. Return no results.

We don't need this comment.  It just says what the code does, not why it does it.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1935
> +    // Do nothing on detached frames.

ditto

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2726
> +    // Frame already detached.
> +    if (!frame() || !frame()->page())
> +        return;

Is this possible?  Also this comment is redundant.

> Source/WebKit/chromium/src/WebFrameImpl.h:427
> +    // Observes for any destruction / page detachment events that might occur
> +    // to the WebCore frame.
> +    OwnPtr<FrameDestructionHelper> m_frameDestructionHelper;

Why not just make WebFrameImpl inherit from FrameDestructionObserver?

> Source/WebKit/chromium/src/WebFrameImpl.h:471
> +    int m_findRequestIdentifier;

Should we initialize this value in the constructor?

> Source/WebKit/chromium/tests/WebFrameTest.cpp:1016
> +    // Detach the frame before finding.
> +    WebFrameImpl* secondFrame = static_cast<WebFrameImpl*>(mainFrame->traverseNext(false));
> +    secondFrame->frame()->willDetachPage();
> +    secondFrame->frame()->detachFromPage();

Rather than doing this manually, we should do this by manipulating the DOM via the API or via JavaScript.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:1061
> +    // Detach the frame between finding and scoping.
> +    WebFrameImpl* secondFrame = static_cast<WebFrameImpl*>(mainFrame->traverseNext(false));
> +    secondFrame->frame()->willDetachPage();
> +    secondFrame->frame()->detachFromPage();

ditto

[Leandro Graciá Gil]

Created attachment 166044 [details]
Patch

[Leandro Graciá Gil]

Comment on attachment 166039 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166039&action=review

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:581
>> +    virtual void willDetachPage()
> 
> willDetachPage should always occur before frameDestroyed, so m_webFrameImpl will always be non-0 here.

Fixed.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1658
>> +    // Detached frame case. Return no results.
> 
> We don't need this comment.  It just says what the code does, not why it does it.

Removed.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1935
>> +    // Do nothing on detached frames.
> 
> ditto

Removed.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:2726
>> +        return;
> 
> Is this possible?  Also this comment is redundant.

I was also expecting that willDetachPage would be called only once, but this is indeed being called again on detached frames during destruction.
Also comment removed.

>> Source/WebKit/chromium/src/WebFrameImpl.h:427
>> +    OwnPtr<FrameDestructionHelper> m_frameDestructionHelper;
> 
> Why not just make WebFrameImpl inherit from FrameDestructionObserver?

Done.

>> Source/WebKit/chromium/src/WebFrameImpl.h:471
>> +    int m_findRequestIdentifier;
> 
> Should we initialize this value in the constructor?

Good point, I missed this. Thanks.

>> Source/WebKit/chromium/tests/WebFrameTest.cpp:1016
>> +    secondFrame->frame()->detachFromPage();
> 
> Rather than doing this manually, we should do this by manipulating the DOM via the API or via JavaScript.

I'm open to suggestions. However, there is an important benefit of doing it this way: we can predict exactly when the willDetachPage method will be called and check all possible situations. Whatever the approach this is something we should keep.

[Leandro Graciá Gil]

(In reply to comment #4)
> Created an attachment (id=166044) [details]
> Patch

Tested again against Chromium's Find* browser_tests.

[Adam Barth]

Comment on attachment 166044 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166044&action=review

> Source/WebKit/chromium/tests/WebFrameTest.cpp:1016
> +    WebFrameImpl* secondFrame = static_cast<WebFrameImpl*>(mainFrame->traverseNext(false));
> +    secondFrame->frame()->willDetachPage();
> +    secondFrame->frame()->detachFromPage();

I would add WebNode::remove to the API and then call:

mainFrame->document().getElementById("some_id_that_you_like").remove();

You might need to throw in a webkit_support::RunAllPendingMessages() but I don't think you need that anymore.

[Build Bot]

Comment on attachment 166044 [details]
Patch

Attachment 166044 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14054031

New failing tests:
http/tests/workers/terminate-during-sync-operation.html

[Leandro Graciá Gil]

(In reply to comment #7)
> (From update of attachment 166044 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166044&action=review
> 
> > Source/WebKit/chromium/tests/WebFrameTest.cpp:1016
> > +    WebFrameImpl* secondFrame = static_cast<WebFrameImpl*>(mainFrame->traverseNext(false));
> > +    secondFrame->frame()->willDetachPage();
> > +    secondFrame->frame()->detachFromPage();
> 
> I would add WebNode::remove to the API and then call:
> 
> mainFrame->document().getElementById("some_id_that_you_like").remove();
> 
> You might need to throw in a webkit_support::RunAllPendingMessages() but I don't think you need that anymore.

Let's hope we don't need RunAllPendingMessages(), since that will trigger the scopeStringMatches operations and create issues for the cases where the frames are detached between the first call (the state reset) and the rest.

Is there any special considerations I should keep in mind in order to detach the main frame with that approach? In fact, how can I set an id for the main frame? The body, perhaps?

[Adam Barth]

You won't be able to detach the main frame using this technique.  It's not clear to me whether you can actually detach the main frame in these scenarios.

[Leandro Graciá Gil]

(In reply to comment #10)
> You won't be able to detach the main frame using this technique.  It's not clear to me whether you can actually detach the main frame in these scenarios.

Should we limit the testing to the other frames with your suggested approach, then?

[Adam Barth]

Yeah, that's probably the thing to do.

[Leandro Graciá Gil]

(In reply to comment #12)
> Yeah, that's probably the thing to do.

Ok. I'll upload a new patch tomorrow. Thanks for the feedback.

[Leandro Graciá Gil]

Created attachment 166213 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 166213 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166213&action=review

Thanks for improving the test.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2332
> +    m_frame = frame;
> +    observeFrame(m_frame);

m_frame is now almost redundant.  FrameDestructionObserver also has a m_frame pointer.  It might be worth adding a FIXME comment about removing m_frame.

[Leandro Graciá Gil]

Created attachment 166263 [details]
Patch

[Leandro Graciá Gil]

(In reply to comment #17)
> Created an attachment (id=166263) [details]
> Patch

FIXME added to make sure we don't forget about this.  We should probably take a look again when rethinking the WebFrameImpl destruction.

[WebKit Review Bot]

Comment on attachment 166263 [details]
Patch

Clearing flags on attachment: 166263

Committed r129910: <http://trac.webkit.org/changeset/129910>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.