Right now, imgIDecoderObserver notifications are sometimes synchronous and sometimes asynchronous. We should only fire events asynchronously so we don't block the event loop as much.

windows.h #defines LoadImage, which can easily break compilation. #undef it in the spots where we can run into this problem.

Several of our tests assume that we always call LoadImage synchronously. This isn't necessarily the case! This fixes the visited_image_loading tests in the style system.

Similarly, for the table background tests.

We sometimes get extra asserts, but that's okay. Mark them up.

This test also assumed that we called notifications synchronously. Fix it.

We don't actually use some of these parameters, so we should just remove them.

We shouldn't restart animation in NotifyProxyListener. First, it doesn't make a lot of sense - restarting animations when we're telling a proxy's listener about its status is just wrong. Second, it's not parallel with the stopping of animation, which is in RemoveProxy. Third, it makes it more difficult to centralize the notification and tracking of image state. Instead, we restart the animation on AddProxy. This patch needs a very hard to write test. In particular, we need to load an animated image, navigate away from that image, run the cycle collector, navigate back to that image, and then somehow detect whether the animation has restarted. Detecting animation is not currently possible; I think it should be a follow-up bug to make it possible and write that test.

imgContainerRequest duplicates the body of NotifyProxyListener, which makes it impossible for us to put its notifications in a single place. Instead of having a separate class, let's make imgRequestProxy capable of creating static copies of itself, and put notification inside it too.

This patch removes all state tracking from imgRequest and imgRequestProxy, and puts it into a separate class, imgStateTracker. This new class is also the caller of all state transitions to imgRequestProxy (this is so we can easily audit the use of state transitions, and ensure we're only calling them when we're supposed to - this is enforced by no longer giving imgRequest friendship of imgRequestProxy, and leaving all the on* functions protected on imgRequestProxy). These state objects are held by imgContainers.

This is the patch that actually fixes this bug. Attachment 451735 [details] [diff] adds a central state tracker for image state, and makes sure it's the only place that actually calls the state notifications. This patch builds on that to make it possible for imgRequestProxys to have their notifications deferred, and an event posted for that notification to happen later. (In the mean time, we still accumulate any state changes that happen.)

My unit tests use loadImage and loadImageWithChannel, and I didn't see a good reason for these (chrome-only) functions to be limited to [noscript].

This patch tests all asynchronous notification. In particular, it does the following: - Ensures all notifications for LoadImage() are sent asynchronously. - Ensures an imgRequestProxy retrieved from LoadImage(), when cloned, sends its clone's listeners notifications asynchronously. - Ensures that a cache hit in LoadImage sends its notifications asynchronously. We also do all the above for LoadImageWithChannel, with the added test that we don't send notifications outside of the channel's onStartRequest/onStopRequest pair (i.e., all imgIDecoderObserver notifications happen between onStartRequest and onStopRequest from the channel passed in to LoadImageWithChannel).

All these patches are currently running through the try server. In the past, I've had some issues with timeouts in the extension manager tests; we'll see what happens this time. Mossop has told me on irc that these patches seem to make some of the extension manager tests take twice as long, which is a perfectly acceptable reason for them to time out, but is obviously an unintended consequence.

Comment on attachment 451736 [details] [diff] [review] step 7: delay all notifications, and asynchronously send them later (when applicable) I guess this is technically an API change, so we should get superreview on it.

Comment on attachment 451737 [details] [diff] [review] step 8: make loadImage and loadImageWithChannel scriptable This is certainly an API change, and so we should get superreview on it.

Comment on attachment 451736 [details] [diff] [review] step 7: delay all notifications, and asynchronously send them later (when applicable) >+ // The following notification functions are protected to ensure that (friend >+ // class) imgStatusTracker is the only class allowed to send us >+ // notifications. >+ >+ // Whether we want notifications from imgStatusTracker to be deferred until >+ // an event it has scheduled has been fired. >+ PRBool GetDeferNotifications() const >+ { >+ return mDeferNotifications; >+ } I think I actually might like DeferNotifications() better than GetDeferNotifications() because it reads better in cases like: if (!DeferNotifications()) { } However it doesn't matter that much.

(In reply to comment #9) > Created an attachment (id=451735) [details] > step 6: centralize the tracking of image load/decode status I love the direction of this patch. One things that scares me about it a little bit though is that it separates construction and initialization of imgContainers, which is not something that I took into account when I implemented it in its current state. Under the patch, I believe that the only methods called on an uninitialized imgContainer are lockImage/unlockImage, which happens to be ok since CanDiscard happens to return false for an uninitialized imgContainer. Still, this is really undefined behavior, and I think we should be more clear about it to avoid future problems. I've filed bug 575390 for this.

Comment on attachment 451703 [details] [diff] [review] step 2.0: Fix the visited image loading tests r=dbaron

Comment on attachment 451705 [details] [diff] [review] step 2.1: fix the table background reftests r=dbaron

Comment on attachment 451706 [details] [diff] [review] step 2.2: mark up the background draw nothing tests as getting another assert r=dbaron

Comment on attachment 451707 [details] [diff] [review] step 2.3: fix the bug 445810 test It seems like it would be simpler to just make three occurrences of the change: - currImageElem.onload = step2; + currImageElem.onload = function() { setTimeout(step2, 0); } instead of making the extra layer of functions so visible (and all the reindenting) so r=dbaron with that Or does the forceDecode really need to be separated by a timeout?

(In reply to comment #23) > Comment on attachment 451707 [details] [diff] [review] > step 2.3: fix the bug 445810 test > > It seems like it would be simpler to just make three occurrences of the change: > > - currImageElem.onload = step2; > + currImageElem.onload = function() { setTimeout(step2, 0); } > > instead of making the extra layer of functions so visible (and all the > reindenting) > > so r=dbaron with that > > Or does the forceDecode really need to be separated by a timeout? Nope! forceDecode forces a synchronous decode.

Updated to reflect dbaron's review comments. Carrying over review.

Unbitrotted, and account for the fact that we always have an mImage - we just might not have initialized it.

Unbitrotted, and a) send notifications synchronously from Clone (I need to file a followup bug to fix this; too many users assume Clone is synchronous); b) send OnStopDecode along with OnStopRequest in SyncNotify, as we do in SendStopRequest.

Test asynchronous notification, and include testing the asynchronous notification of 404 elements, including 404s that start loading while the same resource is being concurrently loaded and is already in the cache. (This bit me hard in the layout/base/tests/bug570378 tests, because precisely that happens in those cases, but only sometimes.)

Comment on attachment 457991 [details] [diff] [review] step 6: centralize the tracking of image load/decode status, v2 Joe walked me through this and while it didn't make me thrilled it seemed to make sense.

Comment on attachment 457991 [details] [diff] [review] step 6: centralize the tracking of image load/decode status, v2 diff --git a/modules/libpr0n/src/imgRequest.cpp b/modules/libpr0n/src/imgRequest.cpp --- a/modules/libpr0n/src/imgRequest.cpp +++ b/modules/libpr0n/src/imgRequest.cpp + // We only want to send onStartContainer once, but we might get multiple + // OnStartContainer calls (e.g. from multipart/x-mixed-replace). Therefore, + // we tell our status tracker about OnStartContainer *after* attempting to + // send the notifications. That way, if we have previously called + // OnStartContainer, the status tracker can notice. This is a nasty bit of trickery (my fault I think) that I think we ought to eliminate. What about having some explicit state in the status tracker marking whether this is our first time through the notification cycle. mFirstTimeThrough or something? - // If we're not multipart, we shouldn't have an image yet - NS_ABORT_IF_FALSE(mIsMultiPartChannel || !mImage, - "Already have an image for non-multipart request"); Why do we have to eliminate this assert? Can't we just change the second condition to !mImage->IsInitialized()? - if (mImage && NS_SUCCEEDED(status)) { + mImage->GetStatusTracker().RecordStopRequest(lastPart, status); . - // Flag that we loaded the image - mImageStatus |= imgIRequest::STATUS_LOAD_COMPLETE; - + // If the request went through, update the cache entry size. Otherwise, + // cancel the request, which removes us from the cache. + if (NS_SUCCEEDED(status)) { Shouldn't we still be checking mImage->IsInitialized()? The request can succeed from necko's perspective, but it could have been an invalid image type. diff --git a/modules/libpr0n/src/imgStatusTracker.cpp b/modules/libpr0n/src/imgStatusTracker.cpp new file mode 100644 --- /dev/null +++ b/modules/libpr0n/src/imgStatusTracker.cpp + // make sure that observer gets an OnStopDecode message sent to it + if (!(mState & stateRequestStopped)) { + aProxy->OnStopDecode(aStatus, nsnull); + } + } + + // make sure that observer gets an OnStopRequest message sent to it + if (!(mState & stateRequestStopped)) { + aProxy->OnStopRequest(PR_TRUE); + } These two comments made more sense in their old context, before this operation was decomposed into its own method. So - unnecessary comment? *ducks* + if (!(mImageStatus & imgIRequest::STATUS_LOAD_PARTIAL)) Under your patch, STATUS_LOAD_PARTIAL never actually gets set. This begs the question - do we need it at all? +void +imgStatusTracker::Notify(imgRequestProxy* proxy) +{ I've always thought this was a really crappy name for this method. Can we give it one that says more about what it does? BringObserverUpToSpeed() or something? +void +imgStatusTracker::Cancel() +{ + if (!(mImageStatus & imgIRequest::STATUS_LOAD_PARTIAL)) + mImageStatus |= imgIRequest::STATUS_ERROR; +} I'd prefer this to be named something that reflects its book-keeping nature (Cancel() makes me think it does something more drastic). Maybe RecordCancel()? I'm flexible on this if you'd like to keep calling it "Cancel()". diff --git a/modules/libpr0n/src/imgStatusTracker.h b/modules/libpr0n/src/imgStatusTracker.h new file mode 100644 --- /dev/null +++ b/modules/libpr0n/src/imgStatusTracker.h +#include "nsAutoPtr.h" is this include necessary? + stateDecodeStopped = PR_BIT(3), + stateFrameStopped = PR_BIT(4), + stateContainerStopped = PR_BIT(5), The use of stateDecodeStopped and stateContainerStoppped in this patch is incorrect. OnStopDecode always goes with OnStopRequest as a partner (signalling the success/failure of the _load_ since OnStopRequest can't do that given its signature), and OnStopContainer is our poor man's OnStopDecode. This patch currently has two bugs in it as a result of this: 1) in Notify(), it fires OnStopDecode() (currently a _load_ notification) based on stateDecodeStopped. However, stateDecodeStopped is triggered by OnStopContainer, a _decode_ notification. This will break when loading and decoding are separated, ie with decode-on-draw. 2) stateContainerStopped is not cleared when all of the rest of the decode notifications are cleared. This means that Notify can fire OnStopContainer (poor man's OnStopDecode) without firing OnStartDecode, putting consumers in a weird state. Notifications mean what they should mean in imgRequest and below. The weird mapping (bundling onStopDecode w/ onStopRequest, and using onStopContainer to represent onStopDecode) takes place in imgRequestProxy and above. See the comment here: http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/src/imgRequest.cpp#782 My suggestion for making this correct: 1) Eliminate stateContainerStopped 2) Make RecordContainerStopped a no-op (this event will be going away soon anyway) 3) Set stateDecodeStopped when imgRequest gets OnStopDecode() (at this level, the translation has not yet occured). 4) In imgStatusTracker, fire OnStopDecode() with OnStopRequest(), and fire OnStopContainer based on stateDecodeStopped We can talk about this more on IRC if this doesn't make sense. + +class imgStatusTracker +{ I'd prefer some sort of high-level documentation here describing what this class is about. Other Comments: -Please test this patch with discarding enabled, and also with discarding+decode-on-draw enabled. A lot of assumptions stop being valid when we move to that territory. -Let's not forget about my earlier comment and bug 575390. Great patch on the whole. I love the direction this is taking us.

(In reply to comment #30) > + // We only want to send onStartContainer once, but we might get multiple > + // OnStartContainer calls (e.g. from multipart/x-mixed-replace). Therefore, > + // we tell our status tracker about OnStartContainer *after* attempting to > + // send the notifications. That way, if we have previously called > + // OnStartContainer, the status tracker can notice. > > This is a nasty bit of trickery (my fault I think) that I think we ought to > eliminate. What about having some explicit state in the status tracker marking > whether this is our first time through the notification cycle. > mFirstTimeThrough or something? The problem with doing that is that it makes SendStartContainer change state. I'd rather all the state-changing happen in Record* and all the notification-sending happen in Send*; further, if we have zero observers, we'll never call SendStartContainer. Basically, I don't see a way to make this better than it is now, though I'm willing to be proven wrong. > - if (mImage && NS_SUCCEEDED(status)) { > + mImage->GetStatusTracker().RecordStopRequest(lastPart, status); > . > - // Flag that we loaded the image > - mImageStatus |= imgIRequest::STATUS_LOAD_COMPLETE; > - > + // If the request went through, update the cache entry size. Otherwise, > + // cancel the request, which removes us from the cache. > + if (NS_SUCCEEDED(status)) { > > Shouldn't we still be checking mImage->IsInitialized()? The request can succeed > from necko's perspective, but it could have been an invalid image type. Yes, sir. This was actually caught by your test in bug 553982, thankfully. Fixed. > These two comments made more sense in their old context, before this operation > was decomposed into its own method. So - unnecessary comment? *ducks* wooo removing comments > + if (!(mImageStatus & imgIRequest::STATUS_LOAD_PARTIAL)) > > Under your patch, STATUS_LOAD_PARTIAL never actually gets set. This begs the > question - do we need it at all? You get to use it in bug 565773, which I have just assigned to you :) > +void > +imgStatusTracker::Notify(imgRequestProxy* proxy) > +{ > > I've always thought this was a really crappy name for this method. Can we give > it one that says more about what it does? BringObserverUpToSpeed() or > something? Actually I disagree; we're notifying the proxy's listener of all things that happened. These notifications would have happened in the usual course of things, except maybe you loaded the image second. > I'd prefer this to be named something that reflects its book-keeping nature > (Cancel() makes me think it does something more drastic). Maybe RecordCancel()? I was thinking this. Done. > +#include "nsAutoPtr.h" > > is this include necessary? Nope, and neither is nsCOMPtr. Both replaced with the includes I actually need. > > + stateDecodeStopped = PR_BIT(3), > + stateFrameStopped = PR_BIT(4), > + stateContainerStopped = PR_BIT(5), > > The use of stateDecodeStopped and stateContainerStoppped in this patch is > incorrect. OnStopDecode always goes with OnStopRequest as a partner (signalling > the success/failure of the _load_ since OnStopRequest can't do that given its > signature), and OnStopContainer is our poor man's OnStopDecode. This patch > currently has two bugs in it as a result of this: > > 1) in Notify(), it fires OnStopDecode() (currently a _load_ notification) based > on stateDecodeStopped. However, stateDecodeStopped is triggered by > OnStopContainer, a _decode_ notification. This will break when loading and > decoding are separated, ie with decode-on-draw. This is actually fixed in later patches, but I've gone ahead and moved the fix to this patch. > My suggestion for making this correct: > > 1) Eliminate stateContainerStopped > 2) Make RecordContainerStopped a no-op (this event will be going away soon > anyway) > 3) Set stateDecodeStopped when imgRequest gets OnStopDecode() (at this level, > the translation has not yet occured). > 4) In imgStatusTracker, fire OnStopDecode() with OnStopRequest(), and fire > OnStopContainer based on stateDecodeStopped All this is done. > +class imgStatusTracker > > I'd prefer some sort of high-level documentation here describing what this > class is about. OK. > -Let's not forget about my earlier comment and bug 575390. Your earlier comment is comment 19, right? I think that's covered completely by bug 575390, but please correct me if I'm wrong.

Addressed review comments.

Updated for the changes to step 6.

Frig. I forgot that OnStopDecode, as of bug 435296, didn't have any calls to imgRequestProxy::OnStopDecode from within imgRequest::OnStopDecode. This caused images loaded by the "image document" (what you get when you navigate directly to an image) to not finish loading on error (throbber just kept throbbing, onload never fired). This patch adds those calls, and fixes this issue. (It's otherwise exactly the same as v3.)

This will also fix bug 579558, I'm told, which is itself probably a blocker. Moving this to betaN, but I think we should try for beta3 or beta4.

Comment on attachment 458546 [details] [diff] [review] step 6: centralize the tracking of image load/decode status, v3.1 w00t - r=bholley

This is almost ready, and we should get it in for beta 3.

Comment on attachment 458462 [details] [diff] [review] step 7: delay all notifications, and asynchronously send them later (when applicable), v3 diff --git a/modules/libpr0n/src/imgLoader.cpp b/modules/libpr0n/src/imgLoader.cpp --- a/modules/libpr0n/src/imgLoader.cpp +++ b/modules/libpr0n/src/imgLoader.cpp - proxy->NotifyListener(); + + // Notify synchronously, because we're already in OnStartRequest, an + // asynchronously-called function. + proxy->SyncNotifyListener(); Hrm. I suppose this will have to be fixed in bug 579122, but let's leave it for now. diff --git a/modules/libpr0n/src/imgRequestProxy.cpp b/modules/libpr0n/src/imgRequestProxy.cpp --- a/modules/libpr0n/src/imgRequestProxy.cpp +++ b/modules/libpr0n/src/imgRequestProxy.cpp - clone->NotifyListener(); + clone->SyncNotifyListener(); Add a comment explaining that this is wrong and giving the followup bug number. I think it's reasonably high priority, because we want to be able to guarantee to consumers that we're not notifying synchronously. + // We don't have an imgRequest, so we can only notify the clone of our + // current state, but we still have to do that asynchronously. + mImage->GetStatusTracker().NotifyCurrentState(this); As mentioned on IRC, one of these patches should have ExtractFrame() copy mStatusTracker over, otherwise this won't do the right thing. Also, this comment seems to imply that this method is only ever called from imgRequestProxy::Clone(). Is that the case? If so, can we make this more explicit? Given that we're now trying to guarantee that we don't fire notifications asynchronously, you should add a comment to that effect in imgContainer::RequestDecode(). Currently it dispatches the worker asynchronously instead of doing any decoding first, but the comment says that's just a snappiness issue. We should make it clear that it's a correctness issue as well. r=bholley with those changes

A late review comment to the status tracker patch: We shouldn't have anything like stateDiscarded (especially considering it's not actually set in RecordDiscard() under the current patch). If we're discarded before notifying, we should just not send decode notifications (what we, in effect, do now). If stateDiscarded were to be actually set in RecordDiscard(), things would break. So make my r+ contingent on fixing that.

(carrying over r+) This fixes all the issues Bobby found, and the issues I found in testing the things I fixed. In particular, this version of the patch makes GetStaticRequest work properly for animated, 404 and regular static images now that all image status is stored inside the imgContainer.

(carrying over r+) This version of the patch integrates the comments Bobby had, and unbitrots it based on the changes in step 6.

This version of the tests includes tests for GetStaticRequest in all of the tested URI types, which are now regular static images, images that return 404, and animated images.

Comment on attachment 458873 [details] [diff] [review] step 9: test asynchronous notification, v3 + // A bitfield that tracks which callbacks have been called. + this.state = 0; ... + + this.status = 0; Please add a comment explaining the difference between these two, and maybe give 'status' a better name (like requestStatus). r=bholley

I ran into an NS_ABORT_IF_FALSE on try which came because we were updating imgProxyRequest::mImage before unrolling the locks and rolling them back into the new image. I tried writing a test for this, but it's currently not possible because of bug 581275.

Comment on attachment 459872 [details] [diff] [review] step 6: centralize the tracking of image load/decode status, v5 Looks correct. r=bholley

Comment on attachment 458872 [details] [diff] [review] step 7: delay all notifications, and asynchronously send them later (when applicable), v4 >+++ b/modules/libpr0n/src/imgLoader.cpp >- proxy->NotifyListener(); >+ // If we're loading off the network, explicitly don't notify our proxy. >+ // We're loading from a channel, which is already asynchronous; we don't >+ // want to make it further asynchronous. >+ if (!newChannel) >+ proxy->NotifyListener(); So if |newChannel|, something will still handle calling those notifications sometime? Might be good to just document here what that something is. >+ // XXX: It looks like the wrong load flags are being passed in... >+ requestFlags &= 0xFFFF; Can we get a bug filed on seeing what's up with that? >+ static_cast<imgRequestProxy*>(*_retval)->NotifyListener(); So every time I see that I think it must be a sync notification. Perhaps better to name the async method AsyncNotifyListener or ScheduleListenerNotification or something? >+ // Explicitly don't notify our proxy. We're loading from a channel, which >+ // is already asynchronous; we don't want to make it further asynchronous. Again, it would be good to document who's going to handle notifying. >+++ b/modules/libpr0n/src/imgRequestProxy.cpp > void imgRequestProxy::NotifyListener() > { >+ if (mOwner) { My quick audit suggests we likely never get here with a null mOwner. Worth a followup bug to nix the null-checks here and elsewhere as needed. >+++ b/modules/libpr0n/src/imgStatusTracker.cpp >+ imgStatusNotifyRunnable(imgStatusTracker status, This really means to pass the imgStatusTracker by value? Of particular concern.... the tracker's SyncNotify uses its mImage, but nothing I see ensures that mImage stays alive until this event fires. Am I just missing something? >+ if (!aProxy->GetDeferNotifications()) I'd rather call the getter DeferNotifications(), I think. The Get looks weird at all the callsites. >+ // This is only useful if you do not have an imgRequest, e.g., if you are a >+ // static request returned from imgIRequest::GetStaticRequest(). Ah. Is that how one ends up with null mOwners? sr- until we sort out that mImage ownership issue.

(In reply to comment #46) > Comment on attachment 458872 [details] [diff] [review] > step 7: delay all notifications, and asynchronously send them later (when > applicable), v4 > > >+++ b/modules/libpr0n/src/imgLoader.cpp > >- proxy->NotifyListener(); > >+ // If we're loading off the network, explicitly don't notify our proxy. > >+ // We're loading from a channel, which is already asynchronous; we don't > >+ // want to make it further asynchronous. > >+ if (!newChannel) > >+ proxy->NotifyListener(); > > So if |newChannel|, something will still handle calling those notifications > sometime? Might be good to just document here what that something is. Sure. This is either necko directly (because we're loading for the first time) or somewhat indirectly (because we're checking if-modified-since). > >+ // XXX: It looks like the wrong load flags are being passed in... > >+ requestFlags &= 0xFFFF; > > Can we get a bug filed on seeing what's up with that? Sure as well, though that is very old code that I just moved around. > >+ static_cast<imgRequestProxy*>(*_retval)->NotifyListener(); > > So every time I see that I think it must be a sync notification. Perhaps > better to name the async method AsyncNotifyListener or > ScheduleListenerNotification or something? I don't feel extremely strongly about the name, but I did like that the thing you'd think to use by default, "NotifyListener," did the right (async) thing. If we had AsyncNotifyListener and SyncNotifyListener, it might not jump out at you that one is right and one is wrong. > >+++ b/modules/libpr0n/src/imgRequestProxy.cpp > > void imgRequestProxy::NotifyListener() > > { > >+ if (mOwner) { > > My quick audit suggests we likely never get here with a null mOwner. Worth a > followup bug to nix the null-checks here and elsewhere as needed. Any imgRequestProxy can have a null mOwner if print preview calls GetStaticRequest on an animated image. > >+++ b/modules/libpr0n/src/imgStatusTracker.cpp > >+ imgStatusNotifyRunnable(imgStatusTracker status, > > This really means to pass the imgStatusTracker by value? Yes, because we're notifying the _current_ status. But you did find a potential problem here, see below. > Of particular > concern.... the tracker's SyncNotify uses its mImage, but nothing I see ensures > that mImage stays alive until this event fires. Am I just missing something? I suspect that finding an example of this falling down is difficult or possibly impossible, because imgRequestProxy also holds a reference to the image, but you're right that we're better safe than sorry. I'll modify imgStatusNotifyRunnable to hold on to a reference to the tracker's mImage. > >+ if (!aProxy->GetDeferNotifications()) > > I'd rather call the getter DeferNotifications(), I think. The Get looks weird > at all the callsites. OK.

(In reply to comment #47) > > >+ if (!aProxy->GetDeferNotifications()) > > > > I'd rather call the getter DeferNotifications(), I think. The Get looks weird > > at all the callsites. > > OK. DefereNotifications() makes it seem like the function is defering the notifications. How about NotificationsDefered() instead?

> How about NotificationsDefered() instead? Sold.

Made all changes you requested. I didn't change the name of NotifyListener for reasons I explained earlier; if you feel strongly, please let me know.

Somehow I forgot to upload this at any earlier point. Just adds some logging to various imgStatusTracker methods.

Comment on attachment 460714 [details] [diff] [review] step 7.1: add some logging to imgStatusTracker r=bholley. Land this stuff already! ;-)

Comment on attachment 460713 [details] [diff] [review] step 7: delay all notifications, and asynchronously send them later (when applicable), v5 sr=bzbarsky

http://hg.mozilla.org/mozilla-central/rev/6bd0e6c7fc94 http://hg.mozilla.org/mozilla-central/rev/e2ca1e2f6e40 http://hg.mozilla.org/mozilla-central/rev/ea5666a40bf1 http://hg.mozilla.org/mozilla-central/rev/005cd62c6689 http://hg.mozilla.org/mozilla-central/rev/807e344fc413 http://hg.mozilla.org/mozilla-central/rev/e007030c3b07 http://hg.mozilla.org/mozilla-central/rev/130bec306f0e http://hg.mozilla.org/mozilla-central/rev/bc90a512d44d http://hg.mozilla.org/mozilla-central/rev/17efdfae5c82 http://hg.mozilla.org/mozilla-central/rev/cbd7ed81d4a8 http://hg.mozilla.org/mozilla-central/rev/505beccaf337 http://hg.mozilla.org/mozilla-central/rev/3da4f3cf80e7 http://hg.mozilla.org/mozilla-central/rev/b8b62b351c09

for the LoadImage #undef, seems like that should be in gfxWindowsSurface.h. Also, we can probably get rid of the windows.h include in gfxWindowsSurface.h by just providing local definitions of HDC and HWND; they're declared via DECLARE_HANDLE in WinDef.h (with DECLARE_HANDLE coming from WinNT.h), so: struct nameHWND__; struct nameHDC__; typedef struct nameHWND__ *HWND; typedef struct nameHDC__ *HDC;

I filed bug 582783 for that.

This seems to cause a Tp hit at least on Mac and Linux so far, and a pretty significant one (5% on Linux). Joe, can you look into whether there are particular pages that get slower, maybe? Do we need a bug to track the hit?

Runnables are really expensive on mac - this was the source of the 17% decode-on-draw regression. See bug 517091. I wonder if this causes us to do enough of them to regress performance. Hopefully not (since that would be a tough thing to fix). I suppose a solution would be to pool them somehow (so that we only ever have one runnable in the queue at once, where that runnable notifies multiple listeners) - similar to imgDiscardTracker. One thing that might suggest it's not the runnables is that the regression is equally large on linux.

I think it's worth doing what I suggest in comment 57 before trying to guess what's going on. If the regression is limited to only some pages, and particularly large on those, we should be able to just profile+debug it.

This bug has been identified as the regression point for a huge hit on rendering performance in the 'satellite' view of 'Google Maps'. i personally observe this on Linux associated with high CPU spikes. A bug has been filed for this: https://bugzilla.mozilla.org/show_bug.cgi?id=585794 (I am not sure this is the correct way to cross reference this bug but I just wanted to make sure it was noticed by people following the aftermath of this landing).

(In reply to comment #59) > I think it's worth doing what I suggest in comment 57 before trying to guess > what's going on. If the regression is limited to only some pages, and > particularly large on those, we should be able to just profile+debug it. Sorry for out of sequence reply, but see my Comment 59 above. This URL takes a big hit: http://maps.google.co.uk/ In 'Satellite view' and when 'zooming'.

Please file a followup bug on that, Bill. :)

(In reply to comment #62) > Please file a followup bug on that, Bill. :) I believe the followup bug is bug 585794.

(In reply to comment #63) > (In reply to comment #62) > > Please file a followup bug on that, Bill. :) > > I believe the followup bug is bug 585794. That's correct.