380115 - Linux 16-bit widget size issues (black rectangle at bottom of long page)

Status: RESOLVED FIXED

(Core :: Graphics, defect, P2)

Description

[Steve Chapel]

Reproducible: Always

Steps to Reproduce:
1. Go to the URL given and scroll to the bottom of the page.

There's a large black rectangle obscuring the text at the bottom of the page. I can reproduce in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070508 Minefield/3.0a5pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070507 SeaMonkey/1.5a. I can certainly search for the regression range and/or try to make a reduced test case if this isn't an obvious duplicate.

Comment 1

[Mats Palmgren (inactive)]

The bug also occurs in SeaMonkey 2007050701 on Linux.
Steve, if you can find the regression range that would be great.  Thanks.

Comment 2

[Steve Chapel]

Regression Range:
The black rectangle does not appear in Firefox trunk build 2006122204
The black rectangle does     appear in Firefox trunk build 2006122304

Comment 3

[Brian Polidoro]

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-12-22+04%3A00%3A00&maxdate=2006-12-23+04%3A00%3A00&cvsroot=%2Fcvsroot
It seems likely to be Bug 364742.

Comment 4

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

Attachment: testcase

The content area here is getting its own widget, because the div has an overflow: auto style on it.  This widget is 26000 pixels too high, and we're trying to draw to it from its top left, thus overflowing the 16-bit signed coordinate space we have to work with in cairo.

Attached is a simple testcase.

Comment 5

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

Attachment: patch

So here's a patch that fixes this.  This switches cairo to 24.8 instead of 16.16.  Note that pixman is still in 16.16 mode; anything that requires fallback that's "big" will most likely be misrendered.  I think that's ok (and shouldn't affect, say, Quartz).  Also this updates the constant for rect conditioning.

Comment 6

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

Comment on attachment 275664 [details] [diff] [review]
patch

This is not the right patch.

Comment 7

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

Attachment: the real patch

The real patch.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 275839 [details] [diff] [review]
the real patch

comment the definition of CAIRO_COORD_MAX to indicate where that magic number comes from

Comment 9

[Stuart Parmenter]

Comment on attachment 275839 [details] [diff] [review]
the real patch

please file a follow up bug to clamp in the X surface?

Comment 10

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

The data URL in the URL field does weird things on linux with this patch applied; I'm not sure where the problem is.

Old original real-world testcase URL:

http://apcmag.com/6070/mozilla_ceo_speaks_out_on_the_future_of_firefox_the_complete_8_000_wor

Comment 11

[Stuart Parmenter]

vlad: whats going on with this bug?

Comment 12

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

This gets fixed, on at least win32, by the followup patch to bug 384681.  I think it should get fixed on at least OSX; linux -might- get fixed, but I think we have 16 bit issues on linux that are deeper than cairo itself.

Comment 13

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

Yeah, this is fixed on win32 and mac.  Still need to check linux.

Comment 14

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

So I just checked linux; everything works fine for me.  But a few other folks (dholbert and others on irc) checked, and they have issues near the bottom of that page.  He was running ubuntu gutsy with the intel driver; I'm running feisty (older) with the intel driver.  I'm not sure what's going on here.

Comment 15

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

Attachment: long testcase with overflow-x/overflow-y

We're doing ok on all platforms as long as the main page is the large one, as in the test case, or even a page with 32k lines consisting of "abcd<br>" in a 36pt font (resulting page is 2.5M pixels high, we render things fine).

However, the apcmag testcase has a div with 'overflow-x: auto; overflow-y: auto;'; adding that to my long testcase causes breakage on linux.

Comment 17

[Bernd Steinhauser]

Sorry, I didn't find this bug before, I found out, that it is the same problem and the differences are just some configure options.

(cairo/xulrunner built with or without glitz support)

Comment 18

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

Attachment: first part of a fix

So this is the first part of a fix -- the old pre-cairo code seemed to just go ahead and pass large values to gtk/gdk, which most likely clamped them internally.  Since we need to create a surface that knows nothing about gdk, we just clamp these widhts/heights to max xlib 16-bit values.

With that patch, stuff starts rendering, but there are still problems -- in particular, the "long testcase" is still half-broken.  If you scroll to the bottom, stuff doesn't update, and if you scroll too quickly sometimes updating stops.  If you scroll back to the top and then start scrolling down then things will render.

We should probably take this initial bit no matter what, but it'll need more work for the final fix.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 303117 [details] [diff] [review]
first part of a fix

Use a helper method to clamp mSize so we don't have to duplicate this code...

Comment 20

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

Michael, could you take this on?  Figuring out what's going beyond that initial bit is probably a few days' worth of work deep in gdk and gtk2 widget code, and I know nothing about that area...

Comment 21

[Michael Ventnor]

(In reply to comment #20)
> Michael, could you take this on?  Figuring out what's going beyond that initial
> bit is probably a few days' worth of work deep in gdk and gtk2 widget code, and
> I know nothing about that area...
> 

What part of gtk/gdk does this touch? It only seems like Cairo touches Xlib to me...
(I only wish I understood the code more myself :( )

Comment 22

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

Cairo itself does, but the underlying problem here is in the gtk2 widget code -- I have no idea how gtk2 handled this case in firefox 2, where a child widget was > 65536 in height.

Comment 23

[Michael Ventnor]

In Firefox 2 did we do the special casing for that size or did GTK do that for us? I'd like to know whereabouts I should be searching...

Comment 24

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

I have no idea :(  From a quick glance at the code, I don't see anything in nsWindow.cpp that clamped things to 64k, so I have no clue how it worked.

Comment 25

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

Optimistically bouncing this over to roc, based on irc conversation.

Comment 26

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

I think we need to fix this, though I don't know how often it will actually come up in real-world browsing.  It might not be common enough?

Comment 27

[Bernd Steinhauser]

(In reply to comment #26)
> I think we need to fix this, though I don't know how often it will actually
> come up in real-world browsing.  It might not be common enough?

Of course I don't know about others, but I run into this problem about once a week.
(And it is not one page that I visit on a regular basis.)

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This fixes it using the wonderful gdk_window_get_internal_paint_info API.

Comment 29

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

Comment on attachment 309253 [details] [diff] [review]
fix

That's way too easy, cheater. ;)

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This isn't testable with the infrastructure currently available.

Comment 32

[Jeff Walden [:Waldo]]

Sure, but eventually we should return to it and do so when that's possible.  If we were to just flat-out ignore it completely, then it'd be -.

Comment 33

[Sylvain Pasche]

I was looking at bug 425394, and the reduction brings me to something like attachment 292672 [details].

I've updated the testcase slightly in attachment 312128 [details]. See bug 425394 comment 4 how to reproduce the issue.

I also tested that testcase on Windows, and the lines are not drawn any more after line 574. Do you happen to know if there's a bug filed for that?

Comment 34

[Steve Chapel]

Is it bug 215055?

Comment 35

[Sylvain Pasche]

yes, looks like it. Thanks for the pointer.