Bug 36901

Summary: RVCT generates incorrect code for method Range::operator== when compiler optimizations are enabled
Product: WebKit Reporter: Daniel Bates <dbates>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, eric, hausmann, laszlo.gombos, loki, manyoso, staikos
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Other   
Attachments:
Description Flags
Patch with manual test case
none
Patch with manual test case none

Description Daniel Bates 2010-03-31 13:43:04 PDT
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>.
Comment 1 Daniel Bates 2010-03-31 15:10:24 PDT
Created attachment 52214 [details]
Patch with manual test case

Not sure how to test this with DRT so I included a manual test.
Comment 2 Darin Adler 2010-03-31 17:01:35 PDT
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.
Comment 3 Daniel Bates 2010-04-01 10:16:20 PDT
(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.
Comment 4 Daniel Bates 2010-04-01 10:22:29 PDT
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 5 Daniel Bates 2010-04-01 15:06:26 PDT
Comment on attachment 52308 [details]
Patch with manual test case

Clearing flags on attachment: 52308

Committed r56940: <http://trac.webkit.org/changeset/56940>
Comment 6 Daniel Bates 2010-04-01 15:06:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Darin Adler 2010-04-01 16:01:55 PDT
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.
Comment 8 Daniel Bates 2010-04-01 16:08:19 PDT
(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.
Comment 9 Daniel Bates 2010-04-04 23:10:21 PDT
(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).
Comment 10 Darin Adler 2010-04-05 08:44:44 PDT
OK, I see. There is no way to test startInSelection true cases with execCommand at this time.