Closed Bug 81078 Opened 23 years ago Closed 23 years ago

UMR: nsTextFrame::GetPosition() when selecting text. - Trunk [@ nsTextFrame::GetPosition]

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Platform:
x86
Windows NT
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.1

People

(Reporter: kinmoz, Assigned: kinmoz)

References

(
URL
)

Details

(Keywords: crash, topcrash, Whiteboard: r/sr=kin/sfraser a=blizzard for moz0.9.1 check in)

Crash Data

I started up Mozilla (with a hompage of www.mozilla.org) under Purify, then 
loaded the URL:

    http://bugzilla.mozilla.org/show_bug.cgi?id=74383

I selected some text on the page and saw several UMR warnings coming from the 
nsTextFrame::GetPosition() method:



[W] UMR: Uninitialized memory read in nsTextFrame::GetPosition(nsIPresContext *,
nsPoint const&,nsIContent * *,int&,int&) {2 occurrences}
        Reading 2 bytes from 0x0013ef28 (2 bytes at 0x0013ef28 uninitialized)
        Address 0x0013ef28 points into a thread's stack
        Address 0x0013ef28 is 96 bytes past the start of local variable 'paintBu
ffer' in nsTextFrame::GetPosition(nsIPresContext *,nsPoint const&,nsIContent * *
,int&,int&)
        Thread ID: 0xfd
        Error location
            nsTextFrame::GetPosition(nsIPresContext *,nsPoint const&,nsIContent
* *,int&,int&) [nsTextFrame.cpp:3343]
            nsTextFrame::GetContentAndOffsetsFromPoint(nsIPresContext *,nsPoint
const&,nsIContent * *,int&,int&,int&) [nsTextFrame.cpp:3380]
            nsFrame::GetNextPrevLineFromeBlockFrame(nsIPresContext *,nsPeekOffse
tStruct *,nsIFrame *,int,signed char) [nsFrame.cpp:2954]
            nsBlockFrame::HandleEvent(nsIPresContext *,nsGUIEvent *,nsEventStatu
s *) [nsBlockFrame.cpp:5343]
            PresShell::HandleEventInternal(nsEvent *,nsIView *,UINT,nsEventStatu
s *) [nsPresShell.cpp:5522]
            PresShell::HandleEvent(nsIView *,nsGUIEvent *,nsEventStatus *,int,in
t&) [nsPresShell.cpp:5434]
            nsView::HandleEvent(nsGUIEvent *,UINT,nsEventStatus *,int,int&) [nsV
iew.cpp:364]
            nsViewManager::DispatchEvent(nsGUIEvent *,nsEventStatus *) [nsViewMa
nager.cpp:2053]
            HandleEvent    [nsView.cpp:67]
            nsWindow::DispatchEvent(nsGUIEvent *,nsEventStatus&) [nsWindow.cpp:7
02]



You might be able to generate these UMRs selecting any text on any page, but 
I haven't tried it.
I should have added that the UMR is generated by the following BIDI code:


    #ifdef IBMBIDI
 =>         while (IS_BIDI_DIACRITIC(text[aContentOffset - mContentOffset]) ) {
              aContentOffset++;
            }
    #endif // IBMBIDI
*** Bug 80328 has been marked as a duplicate of this bug. ***
I think we should change 
#ifdef IBMBIDI
        while (IS_BIDI_DIACRITIC(text[aContentOffset - mContentOffset]) ) {
           aContentOffset++;
        }
#endif // IBMBIDI

to
#ifdef IBMBIDI
        while ((aContentOffset < mContentLength) &&
               IS_BIDI_DIACRITIC(text[aContentOffset - mContentOffset]) ) {
           aContentOffset++;
        }
#endif // IBMBIDI

simon- is that right?
Assignee: ftang → simon
Target Milestone: --- → mozilla0.9.1
I think the bounds check expression should be:

while ((aContentOffset >= mContentOffset &&
        aContentOffset < (mContentOffset + mContentLength)) &&
        IS_BIDI_DIACRITIC(text[aContentOffset - mContentOffset]) ) {
I agree with kin's version of the bounds checking.
reassign back to ftang for review process
Assignee: simon → ftang
Whiteboard: patch exist
fixed and check in
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
It seems that this doesn't fix the problem completely, since you need to use the 
ip[] offsets array to make sure that you don't run off the end of the text[] 
array. The text[] array used for rendering can be shorter than the content 
string because adjacent spaces are collapsed/dropped under certain situations.

Reopening this bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Whiteboard: patch exist
By the way, mjudge and I tried the following patch yesterday, which we believe 
is the right thing to do, but we still get a UMR whenever aContentOffset is 
equivalent to (mContentOffset + mContentLength - 1) which should be a legal 
index according to the ip[] array! Is PrepareUnicode text not filling in the 
last char of the text[] array?

You can see this UMR by selecting some text in a paragraph that has multiple 
wrapping lines. Click in a line, and drag out of the paragraph so you are moving 
the mouse up/down just past the right edge of the paragraph.



Index: nsTextFrame.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/base/src/nsTextFrame.cpp,v
retrieving revision 1.300
diff -u -r1.300 nsTextFrame.cpp
--- nsTextFrame.cpp     2001/05/16 13:40:08     1.300
+++ nsTextFrame.cpp     2001/05/17 17:37:41
@@ -3342,7 +3342,7 @@
 #ifdef IBMBIDI
         while ((aContentOffset >= mContentOffset) &&
                (aContentOffset < (mContentOffset + mContentLength)) &&
-               IS_BIDI_DIACRITIC(text[aContentOffset - mContentOffset]) ) {
+               IS_BIDI_DIACRITIC(text[ip[aContentOffset - mContentOffset]]) ) {

           aContentOffset++;
         }
 #endif // IBMBIDI

Bah, I pasted the wrong diff file above, here's the correct one:


Index: nsTextFrame.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/base/src/nsTextFrame.cpp,v
retrieving revision 1.300
diff -u -r1.300 nsTextFrame.cpp
--- nsTextFrame.cpp     2001/05/16 13:40:08     1.300
+++ nsTextFrame.cpp     2001/05/17 17:50:04
@@ -3340,10 +3340,12 @@
           }
         }
 #ifdef IBMBIDI
-        while ((aContentOffset >= mContentOffset) &&
-               (aContentOffset < (mContentOffset + mContentLength)) &&
-               IS_BIDI_DIACRITIC(text[aContentOffset - mContentOffset]) ) {
-          aContentOffset++;
+        if (aContentOffset >= mContentOffset) {
+          PRInt32 stopOffset = mContentOffset + mContentLength;
+          while (aContentOffset < stopOffset &&
+                 IS_BIDI_DIACRITIC(text[ip[aContentOffset - mContentOffset]]) )
 {
+            aContentOffset++;
+          }
         }
 #endif // IBMBIDI
         aContentOffsetEnd = aContentOffset;
simon- can you review this ?
mkaply- can you drive the review process and check in ?
Assignee: ftang → mkaply
Status: ASSIGNED → NEW
kin said we should keep this bug open even after we check in because there are 
some other issue around.
mkaply- can you ask sfraser to sr and check in this today?
kin: can you sr this?

I talked to kin. Kin's patch is good, so sr=sfraser on it, but this bug needs to 
stay open for the remaining UMR.

It seems that we're exposing a lot of bugs in the BiDi text frame code. Who 
reviewed this on the layout side?

From nsTextFrame.cpp log:
1.291       erik%netscape.com Apr 11 16:32 bug 74946; author=simon@softel.co.il; 
r=erik; sr=attinasi; diffs from IBM  bidi project (e.g. Arabic, Hebrew)
I think the most important part in kin's patch is the ip[] redirect lookup part. 
The rest is performance which does not really impact the correctness. 
Just try to clerify.

mkaply said he will land it today.
mkaply- make sure you DO NOT close this bug after you land it. kin said there 
are more issue here.
Whiteboard: r/sr=kin/sfraser a=blizzard for moz0.9.1 check in
The change from kin is in
Changing milestone since 0.9.1 changes are in.
Target Milestone: mozilla0.9.1 → mozilla0.9.2
FYI: It turns out that my last patch introduced a hang/crash on linux. (Bug
82556). See the patch in 82556 which fixes the crash, and should fix the UMRs
caused by this loop.

I'll verify that it indeed fixes the UMRs today.
I just checked in a fix to the trunk that should hopefully close out this bug:

    mozilla/layout/html/base/src/nsTextFrame.cpp  revision 1.305
Assignee: mkaply → kin
Priority: -- → P3
I just verified with Purify that this is fixed.

FYI, the fix I checked in was for bug 82556:

    http://bugzilla.mozilla.org/showattachment.cgi?attach_id=36095
Status: NEW → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Target Milestone: mozilla0.9.2 → mozilla0.9.1
Adding crash, topcrash keywords and Trunk [@ nsTextFrame::GetPosition] to 
summary for tracking.  This crash has been showing up in the Talkback topcrash 
reports for a couple of days now.  Since this was just fixed, I'll keep any eye 
out for any new crashes after today.
Keywords: crash, topcrash
Summary: UMR: nsTextFrame::GetPosition() when selecting text. → UMR: nsTextFrame::GetPosition() when selecting text. - Trunk [@ nsTextFrame::GetPosition]
*** Bug 82686 has been marked as a duplicate of this bug. ***
Marking verified per last comments.
Status: RESOLVED → VERIFIED
Crash Signature: [@ nsTextFrame::GetPosition]
You need to log in before you can comment on or make changes to this bug.