421885 - Google reader search results have strange red line and broken border around them

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[David Corley]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b5pre) Gecko/2008031004 Minefield/3.0b5pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b5pre) Gecko/2008031004 Minefield/3.0b5pre

When carrying out a search in Google Reader using the latest Firefox 3.0 nightly, the border around the news item displays a red line and is not formatted correctly (border is discontinuous)

Reproducible: Always

Steps to Reproduce:
1.Open google reader
2.Search for any news item
3.When the search results appear, click on any of them to see the broken border

Actual Results:  
The new item appears with a corrupt border

Expected Results:  
No corrupted border and no red lines

I've reproduced this on Windows XP and Windows 2000.

Comment 1

[David Corley]

Attachment: Screenshot of reader fault

Comment 2

[Ria Klaassen (not reading all bugmail)]

It started with this regression range:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1182366840&maxdate=1182369239
but only if I set a minimum font-size (of 13).
There was also a second regression (more red lines), and I'll try to find the range as soon as I have more time.

Comment 3

[Ria Klaassen (not reading all bugmail)]

Second regression range:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1185383820&maxdate=1185385559

Comment 4

[Brian Polidoro]

Dupe of Bug 404502.  

Comment 5

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #4)
> Dupe of Bug 404502.  
> 
I think so, only in that bug the vertical red lines are not shown on the screen shot. I couldn't reproduce them.

Comment 6

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

Is this still an issue? Could possibly have been fixed by bug 403181.

Comment 7

[David Corley]

yup. still seeing it with 20080312 nightly

Comment 8

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

We need some testcase reduction magic here

Comment 9

[Martijn Wargers (dead)]

Attachment: Unminimized testcase

Comment 10

[Martijn Wargers (dead)]

Attachment: testcase

I first thought this was some kind of rounding thing, but the strange thing is, that I see some blue-ish lines (1 horizontal and 1 vertical one), even though I converted the background-images to black and white ones (one consists of horizontal lines, the other one consists of vertical lines).

Comment 11

[Martijn Wargers (dead)]

Attachment: testcase2

None of the black lines you see, should have a blue-ish line accompanying it.

Comment 12

[David Corley]

There's some anti-aliasing issues going on here. Some lines look gray when seen at default zoom. If you increase the zoom level, it's clear those lines are black, and I think they should appear black at default zoom also, no?
The seconds testcase shows this, and the first testcase has VERY strange behaviour when zoom is increased. The lines disappear?

Comment 13

[Martijn Wargers (dead)]

Attachment: screenshots of testcase2 of trunk and branch

Comment 14

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

Hopefully fixed by patch in bug 421069

Comment 15

[David Corley]

When should I see the 421069 patch? The 20080315 nightly, or does it have to wait until Dbaron signs off on it?

Comment 16

[David Corley]

I'm seeing something else strange. Can't be sure if it started with 20080315 nightly, or before.
In the same search results page where I see the red lines, if I click "Help" on the toolbar, then "About Minefield" and drag the "About Minefield" window over the red lines, gaps start appearing in unwanted red lines and the expected blue border around the result. If I then minimize the minefield window, or cover it with another application window, then make it visible again, the gaps are gone. It's either related to this bug, or there are some repainting issues with the "About Minefield window".

Comment 17

[David Corley]

Attachment: Screenshot of repaint issue

Shows the gaps that appear when dragging the "about minefield" window over search a search result in google reader.

Comment 18

[DiGi]

Trouble is with CSS rule background-repeat:repeat-x;

When is it ON then image (line) is aliased (bit blur) AND with red line. When I turn it off then image is small but sharp and without red line...

Comment 19

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

I'm pretty sure this is fixed by bug 421069, which landed yesterday (and stuck)... can someone check and verify?

Comment 20

[DiGi]

Yes it is fixed. Thanks :)

Comment 21

[Michael Lefevre]

I'm not sure if I'm seeing this bug, bug 403181, or something else similar, but I'm afraid that I still get thin red lines appearing in Google reader when using page zoom, using 2008032106 on Win2k.

Comment 22

[Martijn Wargers (dead)]

Yes, I see that too when zooming in. I'm also seeing that with the unminimized testcase. But it seems to me that is not a bug, but intended.
The Google Reader uses a very weird striped background image, so when zooming in, I can imagine that the background image is placed at a different place, making one of the stripes of the background image visible.

But the main problem is fixed, I think, the blurry stripes you see with the attached screenshots. That blurriness is gone, the lines are now sharp.

Comment 23

[DiGi]

I'm sorry, i didn't test zooming (I don't use that). When I zoom in then red lines jumps back. But when I zoom out then whole content disappear... Vista x64

Comment 24

[Michael Lefevre]

I agree the main issue is fixed.

I also agree that the remaining red lines issue I'm still seeing is not serious. However, it is a change in behaviour since Firefox 2, and I rather doubt that the Google folks intend for random red lines to appear in reader.  So, if all the current zooming/red lines bugs are marked fixed, I think there should be a new bug (maybe it's tech evang if it's decided that Firefox is ok now and Google's site is wrong...)

Comment 25

[Martijn Wargers (dead)]

Firefox 2 doesn't support full Zoom, so it's not possible to make that comparison.
I don't see the issue happening when using text Zoom, btw.
But feel free to file a new bug on this.

The whole content disappearing on zoom out is bug 417178.

Comment 26

[DiGi]

It is that bug 417178 - I had left panel hidden so I lost all topics.

Comment 27

[Michael Lefevre]

(In reply to comment #25)
> Firefox 2 doesn't support full Zoom, so it's not possible to make that
> comparison.

I see your point, but I'd say that adding full zoom was a change ;)  I've made myself a note to file a new bug, but might be a few days before I can make a useful report.

Comment 28

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

Bug 403181 should have fixed issues with the incorrect parts of background images leaking in, as long as the background is just one tile. Maybe there's still a problem in the tiled case? Do we have a reduced testcase that shows the remaining issue?

Comment 29

[Jon Henry]

The red line issue does seem fixed as of the latest nightly on OS X. Major improvement!

At the risk of going offtopic, bug 423312 is still making Google Reader an unpleasant experience on the Mac.

Comment 30

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

Moving to P2 for the remaining issue. I think we may need a new testcase and my Windows build is stale, if any of these testcases still show a problem on zooming, or someone can make a new testcase that does, that'd be very helpful...

Comment 31

[Michael Lefevre]

(In reply to comment #28)
> Do we have a reduced testcase that shows the remaining issue?

Not as far as I've seen :(  Testcases in bug 403181, bug 421069 and this bug are all ok for me now (including zoom). It's why I hesitated to file a new bug - I can have a go at making a new reduced testcase, but that probably won't be until after the weekend, and reducing a Google Reader page doesn't look easy...

Comment 32

[Michael Lefevre]

Oops... redoing roc's changes that I undid by mistake

Comment 33

[Michael Lefevre]

Attachment: image for testcase 3

Little image from Google Reader with a bunch of lines

Comment 34

[Michael Lefevre]

Attachment: Testcase 3

This shows the remaining problem for me.   The table cell width is limited to 15px, and the background gif position is set at -32px.  Google's image has 4 sections for different types of line, separated by 1px red lines.  With no zoom, the intended section with blue line is shown and nothing else.  Zooming in (or out) at some zoom levels makes a red line (from one side or the other of the intended image section) show up as well.

Comment 35

[Michael Lefevre]

Given that I got my platform wrong in an earlier comment, I should mention that I'm using 2008032106 on WinXP.  Hope that testcase is useful.

Comment 36

[Michael Lefevre]

By the way, I also see the issue in comment 16. Loading my testcase, zooming to make a red line appear, and then dragging the about box (or another app e.g. windows calculator) over and away from the red line, the repainting of the red line is inconsistent - sometimes bits of the line disappear, sometimes they reappear.

Comment 37

[David Corley]

FWIW, in Minefield 20080322 I don't see the red lines at zoom levels 0,2,6,8 any more. I do see the lines re-appear at +1 (red at top and bottom) and +3(red at left and top) zoom levels.
News item disappears completely at zoom levels +4, +5 and +7 and also at every zoom level below 0.

Comment 38

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

Thanks for the testcase!!!

Comment 39

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

Yeah, so we're obviously taking the DrawTile path here. Ugh.

Comment 40

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

Attachment: fix

This is the fix for bug 403181 but for the tiled path this time. Hopefully the comments explain what's going on.

The one hitch here is that the reftest doesn't currently pass. I think another pixman bug where it's sampling the wrong pixels :-(. We shouldn't really need to disable the OVER->SOURCE optimization for the tmpSurface path, but we do.

Comment 41

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

I posted to the cairo list about the pixman issue.

Comment 42

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

Attachment: fix v2

My understanding of pixman was faulty and we need to take a slightly different approach to get padding right. This patch seems to work well and it's not too complicated. We build an entirely new tile image covering the image data that needs to be tiled, plus any necessary padding. Hopefully the comments explain adequately.

Comment 43

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

Comment on attachment 311936 [details] [diff] [review]
fix v2


Looks fine, but:

>diff -NrpU12 mozilla-trunk.f13044f71bfb/gfx/thebes/src/gfxASurface.cpp mozilla-trunk/gfx/thebes/src/gfxASurface.cpp
>--- mozilla-trunk.f13044f71bfb/gfx/thebes/src/gfxASurface.cpp	2008-03-27 12:42:00.000000000 +1100
>+++ mozilla-trunk/gfx/thebes/src/gfxASurface.cpp	2008-03-27 12:42:00.000000000 +1100
>@@ -58,24 +58,25 @@
> #include <stdio.h>
> #include <limits.h>
> 
> static cairo_user_data_key_t gfxasurface_pointer_key;
> 
> // Surfaces use refcounting that's tied to the cairo surface refcnt, to avoid
> // refcount mismatch issues.
> nsrefcnt
> gfxASurface::AddRef(void)
> {
>     if (mSurfaceValid) {
>         if (mFloatingRefs) {
>+            // XXXroc huh? how can this be right?
>             // eat a floating ref
>             mFloatingRefs--;
>         } else {
>             cairo_surface_reference(mSurface);
>         }

gfx*Surface's refcounts are tied directly to cairo surface refcounts.  Cairo surfaces (all cairo objects) come into life with a refcount of 1, unlike xpcom objects which start at 0 until they're explicitly AddRef'd.  We record that 1 as a floating ref (meaning that the object has been ref'd, but it's not yet known who owns that ref), and whoever calls AddRef first gets "assigned" that ref.  After that, mFloatingRefs goes to 0, and the refcounts match up correctly from then on.

Comment 44

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

Okay, that makes sense.

Comment 45

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

Attachment: fix for checkin

Without the comment that Vlad addressed

Comment 46

[Reed Loden [:reed]]

Checking in gfx/public/nsIRenderingContext.h;
/cvsroot/mozilla/gfx/public/nsIRenderingContext.h,v  <--  nsIRenderingContext.h
new revision: 1.83; previous revision: 1.82
done
Checking in gfx/public/nsRect.h;
/cvsroot/mozilla/gfx/public/nsRect.h,v  <--  nsRect.h
new revision: 1.24; previous revision: 1.23
done
Checking in gfx/src/nsRect.cpp;
/cvsroot/mozilla/gfx/src/nsRect.cpp,v  <--  nsRect.cpp
new revision: 3.25; previous revision: 3.24
done
Checking in gfx/src/thebes/nsThebesImage.cpp;
/cvsroot/mozilla/gfx/src/thebes/nsThebesImage.cpp,v  <--  nsThebesImage.cpp
new revision: 1.81; previous revision: 1.80
done
Checking in gfx/src/thebes/nsThebesImage.h;
/cvsroot/mozilla/gfx/src/thebes/nsThebesImage.h,v  <--  nsThebesImage.h
new revision: 1.31; previous revision: 1.30
done
Checking in gfx/src/thebes/nsThebesRenderingContext.cpp;
/cvsroot/mozilla/gfx/src/thebes/nsThebesRenderingContext.cpp,v  <--  nsThebesRenderingContext.cpp
new revision: 1.51; previous revision: 1.50
done
Checking in gfx/src/thebes/nsThebesRenderingContext.h;
/cvsroot/mozilla/gfx/src/thebes/nsThebesRenderingContext.h,v  <--  nsThebesRenderingContext.h
new revision: 1.15; previous revision: 1.14
done
Checking in gfx/thebes/public/gfxPoint.h;
/cvsroot/mozilla/gfx/thebes/public/gfxPoint.h,v  <--  gfxPoint.h
new revision: 1.15; previous revision: 1.14
done
Checking in layout/base/nsCSSRendering.cpp;
/cvsroot/mozilla/layout/base/nsCSSRendering.cpp,v  <--  nsCSSRendering.cpp
new revision: 3.360; previous revision: 3.359
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/421885-1-ref.xml,v
done
Checking in layout/reftests/bugs/421885-1-ref.xml;
/cvsroot/mozilla/layout/reftests/bugs/421885-1-ref.xml,v  <--  421885-1-ref.xml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/421885-1.xml,v
done
Checking in layout/reftests/bugs/421885-1.xml;
/cvsroot/mozilla/layout/reftests/bugs/421885-1.xml,v  <--  421885-1.xml
initial revision: 1.1
done
Checking in layout/reftests/bugs/reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.421; previous revision: 1.420
done

Comment 47

[Reed Loden [:reed]]

Helps if I catch all the files when landing... :(

Checking in layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp,v  <--  nsTreeBodyFrame.cpp
new revision: 1.351; previous revision: 1.350
done

Comment 48

[Reed Loden [:reed]]

This failed the added reftest on Mac, so I backed it out.

REFTEST UNEXPECTED FAIL: file:///builds/slave/trunk_osx/mozilla/layout/reftests/bugs/421885-1.xml

Comment 49

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

Sigh. The problem here is simply that I forgot to attach the new test images square-top-bottom-32x32.png and square-left-right-32x32.png. In fact I'm not sure how to do that easily, so I'll just re-land this myself.

Comment 50

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

Relanded

Comment 51

[David Corley]

I'm still seeing the red line issue at +1,3,4 zoom levels. Do I need to write a separate bug report for this?
Also, the search result disappears completely when I use any of the - zoom levels, though I suspect that is a separate bug?

Comment 52

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

Did you build your own build with the patch? My landing can't possibly have reached the nightly builds yet.

Comment 53

[David Corley]

Sorry Robert,
My reading of your last post was that the test itself hadn't been updated (i.e., the source was fine, but the mac reftest wasn't passing because the test was outdated). So I thought the only changes were to the test.
Will check again once todays (or tomorrows, if the landing didn't make it) nightly builds are out.

Comment 54

[David Corley]

Verified as ok at all + zoom levels with 20080331 nightly.

Comment 55

[Michael Lefevre]

Also fixed for me: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008033105 Minefield/3.0pre

Thanks roc :)

(In reply to comment #53)
> My reading of your last post was that the test itself hadn't been updated
> (i.e., the source was fine, but the mac reftest wasn't passing because the test
> was outdated). So I thought the only changes were to the test.

That was the reason for the "sigh" in that comment - your understanding of the problem was correct, but the whole thing was backed out because of it (not knowing that that was the issue).

(comment 51)
> Also, the search result disappears completely when I use any of the - zoom
> levels, though I suspect that is a separate bug?

Yes, that's bug 417178