Bug 36901 – RVCT generates incorrect code for method Range::operator== when compiler optimizations are enabled

• Component: HTML Editing
• Status: RESOLVED FIXED

[Daniel Bates]

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>.

[Daniel Bates]

Created attachment 52214 [details]
Patch with manual test case

Not sure how to test this with DRT so I included a manual test.

[Darin Adler]

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.

[Daniel Bates]

(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.

[Daniel Bates]

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.

[Daniel Bates]

Comment on attachment 52308 [details]
Patch with manual test case

Clearing flags on attachment: 52308

Committed r56940: <http://trac.webkit.org/changeset/56940>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Daniel Bates]

(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.

[Daniel Bates]

(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).

[Darin Adler]

OK, I see. There is no way to test startInSelection true cases with execCommand at this time.