As of RVCT 4.0 (build 697), the method Range::operator== is compiled incorrectly with optimizations. In particular, the if-conditional "if (!&a || !&b)" seems to be optimized away. This is problematic because VisibleSelection::toNormalizedRange() can return a PassRefPtr<Range> to a null pointer by line 132 <http://trac.webkit.org/browser/trunk/WebCore/editing/VisibleSelection.cpp?rev=56825#L132>.
Created attachment 52214 [details] Patch with manual test case Not sure how to test this with DRT so I included a manual test.
Comment on attachment 52214 [details] Patch with manual test case It's illegal to dereference a null pointer and make a reference from it. So code that checks: if (!&a || !&b) is not legal C++. The right way to fix this is to restructure our code so we don't rely on this sort of illegal C++. It's OK with me to do this compiler-specific optimization hacking as an interim measure, but it seems like a bad strategy. Much better would be to eliminate this. One simple way to do this would be to eliminate the Range operator== entirely and add a named function (isEqual or areEqual or something along those lines) that takes range pointers.
(In reply to comment #2) > (From update of attachment 52214 [details]) > It's illegal to dereference a null pointer and make a reference from it. So > code that checks: > > if (!&a || !&b) > > is not legal C++. The right way to fix this is to restructure our code so we > don't rely on this sort of illegal C++. It's OK with me to do this > compiler-specific optimization hacking as an interim measure, but it seems like > a bad strategy. Much better would be to eliminate this. I agree we should eliminate this. > One simple way to do this would be to eliminate the Range operator== entirely > and add a named function (isEqual or areEqual or something along those lines) > that takes range pointers. Will add.
Created attachment 52308 [details] Patch with manual test case Removes Range::operator== and Range::operator !=. Adds function areRangesEqual. This patch address all the call sites in the Mac port. I have not verified this for all the other ports. Hopefully, the EWS bots can check this.
Comment on attachment 52308 [details] Patch with manual test case Clearing flags on attachment: 52308 Committed r56940: <http://trac.webkit.org/changeset/56940>
All reviewed patches have been landed. Closing bug.
I forgot to comment on the manual test case in this bug. There are plenty of automated test cases that exercise finding text, so I think this could be automated. LayoutTests/text/find-case-folding.html for example.
(In reply to comment #7) > I forgot to comment on the manual test case in this bug. > > There are plenty of automated test cases that exercise finding text, so I think > this could be automated. > > LayoutTests/text/find-case-folding.html for example. I will look into this.
(In reply to comment #7) > I forgot to comment on the manual test case in this bug. > > There are plenty of automated test cases that exercise finding text, so I think > this could be automated. > > LayoutTests/text/find-case-folding.html for example. We cannot test this with DRT (at least not via document.execCommand("FindString", ...)) as WebCore::Frame::findString is not called with startInSelection := true (*). The call flow has the form: ... => WebCore::executeFindString => WebCore::Frame::findString. In particular, startInSelection is hardcoded to false in method WebCore::executeFindString, <http://trac.webkit.org/browser/trunk/WebCore/editing/EditorCommand.cpp?rev=55566#L399>. In contrast, when you show the find banner in Safari and perform a search, WebCore::Frame::findString is called with startInSelection := true. The call flow has the form: -[WebView(WebPendingPublic) searchFor:direction:caseSensitive:wrap:startInSelection:] => -[WebHTMLView(WebDocumentPrivateProtocols) searchFor:direction:caseSensitive:wrap:startInSelection:] => WebCore::Frame::findString. Notice the value of startInSelection is passed in as a parameter. I cannot see the code of the calling functions of -[WebView(WebPendingPublic) searchFor:direction:caseSensitive:wrap:startInSelection:] (**) to determine how this value is set since the calling functions are outside of WebKit. I am not too familiar with EditorCommand, but we may be able to use similar logic as in (**) to set the value of startInSelection in WebCore::executeFindString. Alternatively, we may consider allowing a user to explicitly specify this parameter. Therefore, a manual test is needed to reproduce the crash/verify the fix. (*) Notice, the crash occurs when we evaluate the second conjunct of the if-condition on line 1389 of Frame.cpp (i.e. "*VisibleSelection(resultRange.get()).toNormalizedRange() == *selection.toNormalizedRange()"), and by the definition of logical AND this occurs when the first conjunct is true (i.e. startInSelection := true).
OK, I see. There is no way to test startInSelection true cases with execCommand at this time.