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.
Created attachment 166039 [details] Patch
(In reply to comment #1) > Created an attachment (id=166039) [details] > Patch Tested against Chromium's Find* browser_tests.
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
Created attachment 166044 [details] Patch
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.
(In reply to comment #4) > Created an attachment (id=166044) [details] > Patch Tested again against Chromium's Find* browser_tests.
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.
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
(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?
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.
(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?
Yeah, that's probably the thing to do.
(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.
Created attachment 166213 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
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.
Created attachment 166263 [details] Patch
(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.
Comment on attachment 166263 [details] Patch Clearing flags on attachment: 166263 Committed r129910: <http://trac.webkit.org/changeset/129910>
All reviewed patches have been landed. Closing bug.