Right now we just modify mCurrentImage and mPendingImage all over the place, and it's a bit confusing. This would help debug bug 516334, and would be nice for bug 512260 too.

Added a patch. This is a pretty significant reworking of nsImageLoadingContent, but it should make things a lot more sane. The big upshot is that all the code that actually modifies the current/pending requests is in one place, so we can have more confidence that we're actually unblocking any time the "current" request goes away. I'm hopeful that this might just fix whatever weirdness we were seeing in bug 516334 by streamlining the unblocking code. Even if it doesn't though, that issue should now be much easier to debug, both because things are easier to understand now and because we now have a choke point that we can watch. I'm also going to rebase my patch in bug 512260 on top of this, which will simplify that a great deal as well. Pushed to try as e4641d5db98b. Jonathan, you're welcome to give this a shot now and see if you still encounter hangs, or you can wait until the try results come back.

Comment on attachment 405718 [details] [diff] [review] patch v1 tryserver builds look good. jfkthame - want to give the patch a try to see if it fixed bug 516334 while we were at it? Flagging bz for review.

Comment on attachment 405718 [details] [diff] [review] patch v1 flagging joe too.

(In reply to comment #2) > (From update of attachment 405718 [details] [diff] [review]) > tryserver builds look good. jfkthame - want to give the patch a try to see if > it fixed bug 516334 while we were at it? Yes, I definitely want to try this - probably won't get time today but I'll report back ASAP.

I'm not happy with the use of default arguments here. There's a reason they were not defaulted before, and that was to make sure that the caller always thinks about them. Notifying when you shouldn't be is a major major bug (think "web page owns your computer" bug), so we don't want to make it too easy to accidentally notify. I'll try to sort through the rest of the changes.

(In reply to comment #2) > jfkthame - want to give the patch a try to see if > it fixed bug 516334 while we were at it? I tried some test runs with this patch... and unfortunately they still hang, with similar frequency (i.e. very sporadic) to previously. I also confirmed that with SetBlockingOnload patched to be a no-op function, the hangs disappear and the test pageset runs to completion, repeatedly. So it does not appear to have affected bug 516334, either for better or for worse.

(In reply to comment #6) > (In reply to comment #2) > > jfkthame - want to give the patch a try to see if > > it fixed bug 516334 while we were at it? > > I tried some test runs with this patch... and unfortunately they still > hang, with similar frequency (i.e. very sporadic) to previously. I also > confirmed that with SetBlockingOnload patched to be a no-op function, the hangs > disappear and the test pageset runs to completion, repeatedly. So it does not > appear to have affected bug 516334, either for better or for worse. Good to know. With this patch I'm much more confident that a current request can't "sneak away" without us unblocking for it. So I'm betting the bug is in imagelib - somebody's firing OnStartDecode but never firing OnStopContainer.

(In reply to comment #7) > somebody's firing OnStartDecode but never firing OnStopContainer. Yes, this appears to be true; see bug 516334 comment #13. Incidentally, I also see some cases where the request isn't really an image at all, although the page used an <img> tag to refer to it. In these cases, we get OnStopContainer without having received a preceding OnStartContainer. Is this harmless, or should we be concerned? Example from my trace: [this=0x1704d11c] seq=00061156 CREATED [this=0x1704d11c] seq=00061157 OnStartRequest [this=0x1704d11c] seq=00061158 OnStopContainer [this=0x1704d11c] seq=00061159 UNBLOCKING spec='file:///Users/jonathan/page_load_test/pages/www.mop.com/tjj.mop.com/calculate@id=217.html' [this=0x1704d11c] seq=00061160 OnStopDecode [this=0x1704d11c] seq=00061161 OnStopRequest [this=0x1704d11c] seq=00064756 UNBLOCKING (null req) [this=0x1704d11c] seq=00064757 UNBLOCKING (null req)

(In reply to comment #8) > Incidentally, I also see some cases where the request isn't really an image at > all, although the page used an <img> tag to refer to it. In these cases, we get > OnStopContainer without having received a preceding OnStartContainer. Is this > harmless, or should we be concerned? Probably not a big deal, and makes sense given the code, but still should be fixed. I've filed bug 521946 for it.

Comment on attachment 405718 [details] [diff] [review] patch v1 I'd really like to see the patch without optional args so I'm reviewing something closer to what will land.

Updated patch to get rid of the use of default arguments. Reflagging bz.

Updated patch to fix some bitrot and a few other things. All is green on try. Flagging bz for review.

This blocks bug 512260, which blocks a blocker (bug 563088). Nominating.

Comment on attachment 453073 [details] [diff] [review] patch v3 >+++ b/content/base/src/nsImageLoadingContent.cpp > nsImageLoadingContent::DestroyImageLoadingContent() > // If we're blocking onload for any reason, now's a good time to stop > SetBlockingOnload(PR_FALSE); SetCurrentRequest(nsnull) will do this anyway, right? Why do we support SetCurrentRequest(nsnull) instead of just having consumers call ClearCurrentRequest directly? I'd prefer the latter... For that matter, why do we have SetPendingRequest/SetCurrentRequest at all? All callers pass null for the former. The only caller that passes non-null for the latter is PendingToCurrent, which could just as easily have the one line of SetCurrentRequest instead of the one line it has. >@@ -576,17 +558,19 @@ nsImageLoadingContent::LoadImage(const n > // Do make sure to drop our existing image, if any >- CancelImageRequests(aNotify); >+ AutoStateChanger changer(this, aNotify); >+ SetPendingRequest(nsnull); >+ SetCurrentRequest(nsnull); Why that change? That's just copy/pasting the guts of CancelmageRequests... > nsImageLoadingContent::UpdateImageState(PRBool aNotify) >+ // XXX - This machinery should be removed after bug 521604. Which has landed, right? Please file a followup bug to remove. > nsImageLoadingContent::UseAsPrimaryRequest(imgIRequest* aRequest, >+ // It makes more sense to call PrepareNextRequest() alone here No, it doesn't. This method really does mean "use this request as the current request right now". Not that it matters that much, since it's only ever called once on a given image content at the moment... But the comment is wrong. >+nsImageLoadingContent::SetBlockedRequest(nsIURI* aURI, PRInt16 aContentDecision) >+ // For the blocked case, we only want to cancel the existing current request >+ // if size is not available. Don't ask me why. Because the web depended on this behavior last I checked. Duplicating the SIZE_AVAILABLE stuff between here and PrepareNextRequest() is ... annoying. I really don't like it. Can we avoid it? >+ // Set the new blocking status before clearing the current request, so that >+ // the notifications that might go out say the right thing. >+ mImageBlockingStatus = aContentDecision; What notifications? ClearCurrentRequest() doesn't send any notifications... >+nsImageLoadingContent::PendingToCurrent() >+{ >+ SetCurrentRequest(mPendingRequest); There's only one callsite. Why do we need this function? Note that the new impl has an extra addref/release compared to what we used to have; I guess that doesn't matter much. >+nsImageLoadingContent::SetCurrentRequest(imgIRequest* aNewCurrent) >+nsImageLoadingContent::SetPendingRequest(imgIRequest* aNewPending) Again, not sure why we need these at all. >+++ b/content/base/src/nsImageLoadingContent.h >+ * Prepare and returns a reference to the "next request". If there's already >+ * a current request "usable current request" (note the SIZE_AVAILABLE bits). >+ * Called when we would normally call SetNextRequest(), but the request was >+ * blocked. There is no SetNextRequest.... What did this mean to say? >+ * Makes the current pending request current, and gets rid of the old current >+ * request. s/current pending/pending/? Again, not sure why we need this. >+ * The 'prepare' variants do exactly the same thing "same" == "clear", not "set", right? > It is assumed that the COMPtr will be >+ * set to an actual request and not to null. Where is this assumed? This doesn't seem to be a reasonable assumption. >+ * This should only be called through one of the other request helper methods. Why? >+ /* The number of nested AutoStateChangers currently tracking our state. */ >+ PRUint32 mStateChangerDepth; How deep can they get? This would pack better as a PRUint8 at the end of the class, if that gives us enough counting-room.

(In reply to comment #14) > > nsImageLoadingContent::UpdateImageState(PRBool aNotify) > >+ // XXX - This machinery should be removed after bug 521604. > > Which has landed, right? Please file a followup bug to remove. > Filed bug 583385 for this. All the other review comments should be dealt with. Pushed to try as b9924369c86d. Flagging bz for review.

Comment on attachment 461729 [details] [diff] [review] patch v4 r=me. It might be worth filing a bug to look into the src_changed thing too, since I'm not sure nsImageFrame actually relies on that anymore... Worth checking.

> It might be worth filing a bug to look into the src_changed thing too, since > I'm not sure nsImageFrame actually relies on that anymore... Worth checking. Filed bug 583491.

Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/859099a10626 Resolving fixed.

(In reply to Boris Zbarsky [:bz] from comment #5) > I'm not happy with the use of default arguments here. There's a reason they > were not defaulted before, and that was to make sure that the caller always > thinks about them. Notifying when you shouldn't be is a major major bug > (think "web page owns your computer" bug), so we don't want to make it too > easy to accidentally notify. Thinking back here, I never understood this. This is about notifying document observers, right? Can you explain or link me to the relevant security issues here?

Document observers called outside a scriptblocker can end up executing script, and if you do that when you're not expecting script to run, all bets are off.