Closed Bug 455105 Opened 12 years ago Closed 11 years ago

IsSolidBorderEdge in nsCSSRendering.cpp ignores foreground borders

Categories

(Core :: Web Painting, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: dbaron, Assigned: zwol)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 1 obsolete file)

I noticed this while looking at bug 455093:

IsSolidBorderEdge in nsCSSRendering.cpp ignores foreground borders.  This means it reads from the alpha component of an uninitialized color variable.  If that happens to be 255, and the alpha component of the 'color' property is not 255, then we'll misrender.
It also ignores border-image, which could also be transparent.
Flags: blocking1.9.1?
OS: Linux → All
Hardware: PC → All
Blocks: 449324
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Assignee: roc → zweinberg
Attached patch (rev 1) patch (obsolete) — Splinter Review
This is a conservative fix - if we're using a border image at all, or if we are using the foreground color at all, we assume the border might be transparent.  For border image this really is the best we can do; the image might not be available yet and we'd have to duplicate a whole lot of the code from DrawBorderImage.  For foreground color we could give a precise answer but only by changing all callers of PaintBackgroundWithSC to provide the relevant nsStyleColor object, which I thought would be too invasive.
Attachment #345179 - Flags: superreview?(roc)
Attachment #345179 - Flags: review?(vladimir)
Attachment #345179 - Flags: superreview?(roc) → superreview+
Comment on attachment 345179 [details] [diff] [review]
(rev 1) patch

+  // opaque would be too much work.  We can't limit this to when the
+  // border image has been loaded, because it might become loaded
+  // between now and when PaintBorder is called.

That last sentence seems inaccurate. If it's loaded now I think it must be loaded for the PaintBorder (and it's only CSS z-ordering which guarantees that the PaintBorder is called after this is called, which is a weak thing to rely on). But it's definitely not worth doing this optimization for border images, so just delete this part of the comment.
(In reply to comment #3)
>
> That last sentence seems inaccurate. If it's loaded now I think it must be
> loaded for the PaintBorder (and it's only CSS z-ordering which guarantees that
> the PaintBorder is called after this is called, which is a weak thing to rely
> on). 

I'm not sure I understand your objection to the sentence.  As long as there's any condition at all where this function is called before PaintBorder for the same element, and the image is not-loaded when this function is called but subsequently loaded when PaintBorder is called, it is a bug to use IsBorderImageLoaded() instead of GetBorderImage() in the predicate, and that's all the comment is trying to say...
IsBorderImageLoaded is testing main-thread-only state that's only going to be manipulated by a separate event on the main thread event loop; this code and PaintBorder will be called within the same event.
Right, the image can't change state between painting the background and painting the border.
Ok, here's a revision with the incorrect sentence removed.
Attachment #345179 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed to mozilla-central: <http://hg.mozilla.org/mozilla-central/rev/7e85c5796676>
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
Oops, I missed the tests on the previous check-in, here they are: <http://hg.mozilla.org/mozilla-central/rev/d61da77977df>
Flags: in-testsuite+
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.