Hang [@ nsRegion::InsertInPlace] with position:fixed

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
Graphics
--
critical
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Jesse Ruderman, Assigned: mats)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla1.9.3a1
hang, testcase, verified1.9.1, verified1.9.2
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking1.9.2 -, status1.9.2 .2-fixed, blocking1.9.1 -, status1.9.1 .9-fixed)

Details

Attachments

(4 attachments)

(Reporter)

Description

7 years ago
Created attachment 423272 [details]
testcase (hangs Firefox)

Hangs in this loop in nsRegion::InsertInPlace:

  while (aRect->y == mCurRect->prev->y && aRect->x < mCurRect->prev->x)
    mCurRect = mCurRect->prev;

In this hang, mCurRect->prev->prev == mCurRect, and it just keeps looping between the two.
(Reporter)

Comment 1

7 years ago
Created attachment 423273 [details]
stack trace and some debugging info
(Reporter)

Updated

7 years ago
Duplicate of this bug: 541868
Ooof. Assuming the author isn't around, we need someone to dig into the region code and figure this out. Let's try Mats :-).
Assignee: nobody → matspal
blocking2.0: --- → ?

Comment 4

7 years ago
FYI: I am still around, but I think I forgot all the details over these years good enough :) Have not compiled Mozilla since switch to mercurial.
I will also look at the code, but Mats should be able to identify problem not worse than me.

Comment 5

7 years ago
The idea is that rectangles in the region list are linked in the double-linked list ordered first by y and then by x coordinate of left-upper corner of rectangle. mRectListHead is special dummy head rectangle initialized to point to self when list is empty (thus saving checks for NULL next/prev pointers). 

mRectListHead.y is set to PR_INT32_MAX when you are look for smaller y values. Thus it works as a sentinel that protects infinitely wrapping around when traversing list. 

Similarly mRectListHead.y is set to PR_INT32_MIN when you are look for larger y values.

I think the problem is in fact that original code never assumed that you will insert rectangle for which any of coordinates is one of PR_INT32_MIN or PR_INT32_MAX - it was designed for OS/2 just to overcome 16bit physical pixel limitation and I have not thought that it will be used with logical 32 twips that reach min/max values of 32bit numbers. In such case setting stopping limit in mRectListHead.y will not work. 

You have to explicitly check for:

mCurRect->prev != &mRectListHead

in all while loops

Hope this helps

The
(Reporter)

Comment 6

7 years ago
Created attachment 423369 [details]
testcase 2 (also hangs Firefox)

Comment 7

7 years ago
As it will take some time until I will be able to build Mozilla source code again, can someone post the values of mRectListHead and aRect rectangles (x, y, width, height) so I can look what is wrong in algorithm.

One possible problem is that when setting up sentinel values in mRectListHead only y is initialized with PR_INT32_MAX / PR_INT32_MIN (e.g. lines 352, 364, 377, 385). To cover all edge cases actually the same values must be set also for x coordinate.

Comment 8

7 years ago
After little pondering I am almost sure that also setting x sentinel values should fix the problem by preventing mCurRect being initialized with address of mRectListHead and then endlessly looping. Mats, can you please check if this will help:

line 352: Add mRectListHead.x = PR_INT32_MAX;
line 364: Add mRectListHead.x = PR_INT32_MIN;
line 377: Add mRectListHead.x = PR_INT32_MAX;
line 385: Add mRectListHead.x = PR_INT32_MIN;
(Assignee)

Comment 9

7 years ago
Thanks Dainis, that's very helpful.  I agree with you suggested fixes, the
last two can be "Replace" rather than "Add" though.  I will do a review
of the rest of this file as well for similar problems.  Patch coming...

There's also the issue of why we are given an nsRect with y = PR_INT32_MIN
because that shouldn't happen, since it's outside the range
nscoord_MIN .. nscoord_MAX.  I've tracked this down to nsLayoutUtils,
I'll fix that too.

Third issue, nsRegion.cpp doesn't handle NS_COORD_IS_FLOAT, but I guess
that's known.
Status: NEW → ASSIGNED
(Assignee)

Comment 10

7 years ago
Created attachment 423603 [details] [diff] [review]
Patch rev. 1

Fix the above three issues.  It fixes the hang for both test cases.
The NSToCoordFloorClamped changes in nsLayoutUtils fixes the out-of-range
values for the first test case.  I haven't tracked down the cause for the
second test case, but it doesn't seem worth the time fixing all of those...
assuming we'll move to using float nscoord in near future.
Attachment #423603 - Flags: review?(roc)
(Assignee)

Comment 11

7 years ago
Comment on attachment 423603 [details] [diff] [review]
Patch rev. 1

Dainis, please have a look at the nsRegion changes as well if they make
sense to you.  Thanks.
Attachment #423603 - Flags: review?(jonitis)

Comment 12

7 years ago
Looks good!

Updated

7 years ago
Attachment #423603 - Flags: review?(jonitis) → review+
Comment on attachment 423603 [details] [diff] [review]
Patch rev. 1

wonderful, thanks
Attachment #423603 - Flags: review?(roc) → review+
(Assignee)

Comment 14

7 years ago
http://hg.mozilla.org/mozilla-central/rev/f155f3050875
http://hg.mozilla.org/mozilla-central/rev/5f631340007c
Status: ASSIGNED → RESOLVED
blocking2.0: ? → ---
Last Resolved: 7 years ago
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
(Assignee)

Updated

7 years ago
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
(Assignee)

Updated

7 years ago
Attachment #423603 - Flags: approval1.9.2.1?
Attachment #423603 - Flags: approval1.9.1.9?
blocking1.9.1: ? → -
blocking1.9.2: ? → -
status1.9.1: --- → wanted
status1.9.2: --- → wanted
Attachment #423603 - Flags: approval1.9.2.2?
Attachment #423603 - Flags: approval1.9.2.2+
Attachment #423603 - Flags: approval1.9.1.9?
Attachment #423603 - Flags: approval1.9.1.9+
Comment on attachment 423603 [details] [diff] [review]
Patch rev. 1

a=beltzner for 1.9.1 and 1.9.2

Comment 16

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/39676a222968
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/956e710bd1bf
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d9d8c0372d4e
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3c8ea850ffb4
status1.9.1: wanted → .9-fixed
status1.9.2: wanted → .2-fixed
Verified for 1.9.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.9pre) Gecko/20100311 Shiretoko/3.5.9pre (.NET CLR 3.5.30729) and the first attached testcase. It no longer hangs 1.9.1. The second case doesn't hang 1.9.1 pre-patch at all.

Verified for 1.9.2 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.2pre) Gecko/20100311 Namoroka/3.6.2pre (.NET CLR 3.5.30729). The second testcase doesn't hang pre-fix 1.9.2 either.
Keywords: verified1.9.1, verified1.9.2
You need to log in before you can comment on or make changes to this bug.