445810 - Removing -moz-border-image style does not unpaint the offending region

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, you should not see any red with the testcase, but you do with current trunk build.
Clicking on the region makes the red blocks go away.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, I stuck in a comment into nsImageLoader because I was a *little* suspicious of the code, and it looks like we'll have to change that code to invalidate too:

http://hg.mozilla.org/mozilla-central/index.cgi/diff/2bf42512916d/layout/base/nsImageLoader.cpp

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hmm.  I thought removing the early return there would fix this, but it doesn't.

That said, the testcase only shows the bug for me some of the time.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(And I'd note that a FrameNeedsReflow followed by an Invalidate is exactly what nsCSSFrameConstructor::ProcessRestyledFrames does.)

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, the problem is in the lack of code in nsStyleBorder::CalcDifference, though I probably want to make the other change as well...

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

I should have caught a bunch of this when reviewing...

Comment 6

[Rob Arnold [:robarnold]]

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

>+void
>+nsPresContext::StopBackgroundImageFor(nsIFrame* aTargetFrame)
>...
>+void
>+nsPresContext::StopBorderImageFor(nsIFrame* aTargetFrame)
>...

Since these two methods have nearly identical bodies, can we merge them together as was done with LoadBorderImage/LoadImage?

Otherwise looks good.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

>+++ b/layout/base/nsFrameManager.cpp
>+        // Since the CalcDifference call checked whether the new border
>+        // image was loaded in order to determine whether to do a
>+        // reflow, we need to call LoadBorderImage now so that the frame
>+        // gets notified if the image loads.

I'm not sure I follow this comment (or the need for the code that follows it).  Can you explain?

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So there's existing code in nsHTMLReflowState to prevent the following race condition:
 * do a reflow
   + image not loaded yet, so we use the border widths appropriate for no image
 * image loads
 * do paint, and create an nsImageLoader that will notify us when the image loads

This code is to prevent a second race condition:
 * process a style change
   + image we're changing to is not loaded yet, and this means there is
     no change in GetActualBorder()
 * image loads (which changes the result of GetActualBorder())
 * we do a paint, creating an nsImageLoader to notify us when the image loads


To prevent these race conditions, we need to ensure that we've created an nsImageLoader once we've used the result of GetActualBorder() (e.g., Reflow or CalcDifference).

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

>+++ b/layout/base/nsFrameManager.cpp
>+        const nsStyleBorder* oldBorder = oldContext->GetStyleBorder();
>+        const nsStyleBorder* newBorder = newContext->GetStyleBorder();
etc

I wonder whether there's a way to refactor this a bit better with the background image case.  At least an inline function that takes two images and handles all the URI-getting and comparing?

>+        // Since the CalcDifference call checked whether the new border
>+        // image was loaded in order to determine whether to do a
>+        // reflow, we need to call LoadBorderImage now so that the frame
>+        // gets notified if the image loads.

Perhaps:

  Since the CalcDifference call depended on the result of GetActualBorder() and that result depends on whether the image has loaded, start the image load now so that we'll get notified when it completes loading and can do a restyle.  Otherwise, the image might finish loading from the network before we start listening to its notifications, and then we'll never know that it's finished loading.

With that and Rob's changes, sr=bzbarsky.

If it's at all possible, can we add some tests here?

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Addressing comments, and with test.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fixed:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/7370786111c2

Martijn, could you confirm that both problems (this bug and bug 446338) you found are actually fixed by this patch?

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, changing from in-testsuite+ to in-testsuite?, since I added some tests, but they don't cover what this bug report was originally about since we don't yet have a way to test invalidation.

Comment 14

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a2pre) Gecko/2008072703 Minefield/3.1a2pre

However, bug 446338 doesn't seem to be fixed (although better), so I'll reopen that one.