390898 - Crash when dragging selection [@ fbFetchPixel_a8r8g8b8][@ fbFetchPixel_x8r8g8b8] [64bit]

Status: RESOLVED FIXED

Description

[Mats Palmgren (:mats)]

Crash when dragging selection [@ fbFetchPixel_a8r8g8b8]

STEPS TO REPRODUCE
1. click-and-drag to make a selection
2. click on it and drag it around for a bit
3. repeat

BUILDS AND PLATFORMS TESTED
Bug occurs in a local Firefox debug build on Linux x86_64 (Ubuntu 7.04).
Bug does NOT occur in a Firefox nightly build (32-bit) on the same host.
Not sure if it's a 64-bit issue, it could also be that my local build
uses native theme also.

Comment 1

[Mats Palmgren (:mats)]

Created attachment 275216 [details]
stack ("bt" and "bt full")

Comment 2

[Mats Palmgren (:mats)]

Created attachment 275217 [details]
stack + some data

FWIW, it appears as if the gfxContext and surface objects are valid...

Comment 3

[Mats Palmgren (:mats)]

Created attachment 275218 [details]
Test page

Easily reproducible on this content.  I can reproduce it on several other
pages too so it's not specific to this content.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Can anyone reproduce this with 32 bit linux?  I don't have easy access to a 64-bit machine, though I can try with vmware.

Comment 5

[Sylvain Pasche]

Seems to be 64bit related. I couldn't reproduce with your testpage, but when dragging a selection containing an image.

in fbFetchTransformed, line 3890 of gfx/cairo/libpixman/src/pixman-compose.c

  b = bits + (y1)*stride;

I'm seeing a negative y1 (-1), which makes b point outside of the buffer.

Comment 6

[Mats Palmgren (:mats)]

Created attachment 275912 [details] [diff] [review]
fix?

It looks like there are some MOD()'s missing -- which clamps the values
to valid boundaries.  We do it in the
"if (pict->common.repeat == PIXMAN_REPEAT_NORMAL)" block, but not in the
else-block:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/gfx/cairo/libpixman/src/pixman-compose.c&rev=1.5&root=/cvsroot&mark=3753-3758,3817-3822,3890,3956#3720

Comment 7

[Mats Palmgren (:mats)]

FWIW, Select All (CTRL+A) on http://www.mozilla.org/projects/minefield/
and then drag that selection is 100% reproducible crash for me.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 275912 [details] [diff] [review]
fix?

So let's take this to avoid the crash for now, but I'm not sure if this is the right fix.

The case above is PIXMAN_REPEAT_NORMAL, which means just normal repeating.  We hit the else case for PIXMAN_REPEAT_NONE, as well as PIXMAN_REPEAT_PAD and PIXMAN_REPEAT_MIRROR.  Doing MOD for all 3 cases doesn't seem to be correct -- and in fact, it's probably not correct for any of them; e.g. for NONE, we really shouldn't be seeing the wrong coordinates at all.

However, like I said, let's bandaid, and I'll send this upstream for discussion.

Comment 9

[Frederic Bezies]

Looks like my bug 390611 is kinda duplicate of this one.

Adding duplicate on my old bug (opened since 12 days) showing a crash with thunderbird / SM MailNews.

Comment 11

[Mats Palmgren (:mats)]

I added a XXX comment before the added blocks and checked in:
mozilla/gfx/cairo/libpixman/src/pixman-compose.c 	1.6 

-> FIXED

Comment 12

[Steve England [:stevee]]

This (or bug 391243) has caused bug 392170.

Comment 13

[Mats Palmgren (:mats)]

I backed out this since it caused crashes on Windows (bug 392214).

Comment 14

[Frederic Bezies]

Is there any hope to fix again this bug ? Because seeing crash on a single drag and drop is very annoying and simply kills usability. Too bad it crashes on the worse OS family.

Sorry for the spam.

Comment 15

[Mats Palmgren (:mats)]

Created attachment 278524 [details]
Trace log

FWIW, the transform sometimes makes the 'v.vector' values become negative.
I also saw some cases where they were still positive after the transform
but less than v.vector[2] / 2 so they became negative here instead:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/gfx/cairo/libpixman/src/pixman-compose.c&rev=1.7&root=/cvsroot&mark=3715-3716#3700

Comment 16

[Mats Palmgren (:mats)]

Vlad, did you get any response from the Cairo folks on this?

Comment 17

[Frederic Bezies]

It seems it kills also drag and drop in places organizer.

It will be great to see this fixed for Gran Paradiso M9, don't you think so ?

Comment 18

[Frederic Bezies]

It seems it kills also printing in linux.

I did a debug build, and try to print gmail.com start page.

Firefox hangs, and when I typed bt in gdb, I got this :

#0  0x00002b773a1cbb03 in fbFetchPixel_a8r8g8b8 (image=0x1a95020, 
    bits=0x401a94d80, offset=0, indexed=0x0) at pixman-compose.c:705
Cannot access memory at address 0x7fff71b0c138

Comment 20

[Martin Stránský [:stransky]]

Created attachment 286274 [details] [diff] [review]
proposed patch

This bug is caused by different size of pointer and integer on 64bit arches, when -1 in 32bit integer != -1 in 64bit integer. Looks partially fixed but there're still some unfixed places...

Comment 21

[Frederic Bezies]

I tested this patch with both firefox and thunderbird source code. And it was wonderful to see that I can do some drag and drop again. For the last two months or so, a single drap and drop meant immediate crash.

I'm using Ubuntu 7.10 AMD64.

Comment 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 286274 [details] [diff] [review]
proposed patch

This looks fine, though it needs to be upstreamed to pixman as well.  Do you have an idea of what/where the 'missing places' are, or are you just assuming that there would be some?

Comment 23

[Martin Stránský [:stransky]]

The missing places are covered by the attached patch. 

Iteration via. multiplication ((y2)*stride) is already used in the pixman code but not everywhere...

Comment 24

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 286274 [details] [diff] [review]
proposed patch

Looks fine then; let's see if we can get this in for M9, I'll check it in if we can.

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 286274 [details] [diff] [review]
proposed patch

a=endgame drivers for M9

Comment 26

[Stuart Parmenter]

checked in.

Comment 27

[David Baron :dbaron: ⌚️UTC-8]

Does this need to be in a list of local patches to cairo so it doesn't get lost when we upgrade?

Comment 28

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

No need; I'm going to do a sync post-M9 and I usually do a diff first from the last time I did a cairo update -- so I see all that went in.