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.

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

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.

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

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

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

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 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?

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 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?

Addressing comments, and with test.

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?

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.

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.