553565 - Calling imgRequestProxy::RemoveFromLoadGroup synchronously may cause problems

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

https://bugzilla.mozilla.org/show_bug.cgi?id=535887#c0

Comment 1

[Joe Drew (not getting mail)]

This almost always results in us calling functions on deleted objects -> thus [sg:critical?]

Comment 2

[Damon Sicore (:damons)]

Joe, ETA?

Comment 3

[Joe Drew (not getting mail)]

Attachment: call NotifyProxyListener asynchronously

This makes most, but not all, notifications in LoadImage() asynchronous. In particular, we might get callbacks in error conditions (via CancelAndAbort()), and probably in the load group addition too (though that's probably expected).

Couple of questions remain for me:
* Is this safe generally? I've pushed to try, but I'm not convinced we have great test coverage of this.
* How can I write a test that tests this functionality? Must I write a C++ unittest?
* Is it safe to leave the image in the load group even if it's finished? We'll call NotifyProxyListener (and thus RemoveFromLoadGroup) eventually, but there will be a period where we're in it and we "shouldn't" be. (The alternative is to make the addition to the load group asynchronous too.)
* Do we need to go further, and make e.g. CancelAndAbort calls asynchronous? We call it after we've added our request to the load group, and it calls nsIRequest::Cancel().

Comment 4

[Boris Zbarsky [:bzbarsky]]

Joe, is this something I can take a few days to a week to get to, or do I need to bump it to the head of the queue?

Comment 5

[Joe Drew (not getting mail)]

Take the time you need. After thinking it over, I'm not even convinced this is [sg:crit].

Comment 6

[Joe Drew (not getting mail)]

Comment on attachment 443157 [details] [diff] [review]
call NotifyProxyListener asynchronously

got _all manner_ of test failures - and crashes - from this on try. There are definitely consumers who need at least some subset of notifications to happen synchronously. :(

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ah, I hadn't realized how small the patch was.

So one problem is that the patch makes it possible to see notifications out of order.  e.g. if an image load is in progress and I create a new imgIRequest for the same image, and the stop is queued already.... I'll get the stop before the newly-async start, right?

Comment 8

[Joe Drew (not getting mail)]

I do not yet have a solution for this bug, but I've been working on a set of patches that iterate us towards a solution. I'm going to get those reviewed while I work on the final fix.

Comment 9

[Joe Drew (not getting mail)]

Attachment: step 1: remove unused parameters in imgRequestProxy::on*

Comment 10

[Joe Drew (not getting mail)]

Attachment: step 2: don't restart animation in NotifyProxyListener

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.

Comment 11

[Joe Drew (not getting mail)]

Attachment: step 3: remove imgContainerRequest

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.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Joe, you have tested print preview and animated gifs/pngs with the patch (step3)?
https://bug487667.bugzilla.mozilla.org/attachment.cgi?id=401186

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(note, we don't use gecko's print preview on OSX)

Comment 14

[Joe Drew (not getting mail)]

(In reply to comment #12)
> Joe, you have tested print preview and animated gifs/pngs with the patch
> (step3)?
> https://bug487667.bugzilla.mozilla.org/attachment.cgi?id=401186

Yep - works great.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 444723 [details] [diff] [review]
step 3: remove imgContainerRequest

Not really code I'm familiar with, except imgContainerRequest.
Good to see that going away.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 444721 [details] [diff] [review]
step 1: remove unused parameters in imgRequestProxy::on*

Yes please.

Comment 17

[Joe Drew (not getting mail)]

Attachment: step 4: centralize the tracking of image load/decode status

This patch removes all state tracking from imgRequest and imgRequestProxy, and puts it into a separate class, imgRequestStateTracker. 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), but I'm not 100% happy with how I've made the API of imgRequestStateTracker, so I'd be happy to have people suggest other ways of doing the same thing.

Comment 18

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 444722 [details] [diff] [review]
step 2: don't restart animation in NotifyProxyListener

># HG changeset patch
># Parent 7bcc465d67373c1589a2dbe717c18e4c9c12a3bc
>
>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
>@@ -170,16 +170,23 @@ nsresult imgRequest::AddProxy(imgRequest
> 
>   // If we're empty before adding, we have to tell the loader we now have
>   // proxies.
>   if (mObservers.IsEmpty()) {
>     NS_ABORT_IF_FALSE(mKeyURI, "Trying to SetHasProxies without key uri.");
>     imgLoader::SetHasProxies(mKeyURI);
>   }
> 
>+  // If we don't have any current observers, we should restart any animation.
>+  if (mImage && !HaveProxyWithObserver(proxy) && proxy->HasObserver()) {

Perhaps we shouldn't exclude 'proxy' from our HaveProxyWithObserver function.
HaveProxyWithObserver also isn't a very good name, because if you read it intuitively
it sounds like it should check whether we have 'proxy' with an observer. Instead, it checks
whether we have any other proxies with observers. It might also be good to have a little story about
where this piece of code came from.

>+    LOG_MSG(gImgLog, "imgRequest::AddProxy", "resetting animation");
>+
>+    mImage->ResetAnimation();
>+  }
>+

Joe says that we shouldn't need to have the UnlessExists here. Maybe we should assert that.

>   return mObservers.AppendElementUnlessExists(proxy) ?
>     NS_OK : NS_ERROR_OUT_OF_MEMORY;
> }
> 
> nsresult imgRequest::RemoveProxy(imgRequestProxy *proxy, nsresult aStatus, PRBool aNotify)
> {
>   LOG_SCOPE_WITH_PARAM(gImgLog, "imgRequest::RemoveProxy", "proxy", proxy);
> 
>@@ -290,22 +297,16 @@ nsresult imgRequest::NotifyProxyListener
>     nsIntRect r;
>     mImage->GetCurrentFrameRect(r);
>     proxy->OnDataAvailable(frame, &r);
> 
>     if (mState & stateRequestStopped)
>       proxy->OnStopFrame(frame);
>   }
> 
>-  if (mImage && !HaveProxyWithObserver(proxy) && proxy->HasObserver()) {
>-    LOG_MSG(gImgLog, "imgRequest::NotifyProxyListener", "resetting animation");
>-
>-    mImage->ResetAnimation();
>-  }
>-
>   // The "real" OnStopDecode - Fix this with bug 505385.
>   if (mState & stateDecodeStopped)
>     proxy->OnStopContainer(mImage);
> 
>   if (mState & stateRequestStopped) {
>     proxy->OnStopDecode(GetResultFromImageStatus(mImageStatus), nsnull);
>     proxy->OnStopRequest(mHadLastPart);
>   }

Comment 19

[Joe Drew (not getting mail)]

I've run into another problem with my 'call everything asynchronously' patch, which basically amounts to the load group that the document is listening to finishing before we send any decode notifications. This happens because we use our own load group in LoadImage instead of using the load group passed in by the caller.

Maybe we should add the new load group we create as a sub-load group of the passed-in one?

Comment 20

[Joe Drew (not getting mail)]

Attachment: step 5: delay all notifications, and asynchronously send them later

Maybe we just shouldn't send the notifications doubly asynchronously when we have to load off the network?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Do we not add the imgRequestProxy to the passed-in loadgroup?

Comment 22

[Joe Drew (not getting mail)]

I thought not, but it looks like we do. More debugging necessary here; that load group shouldn't be able to think it's done until all the imgRequestProxys get their OnRequestStop, and that can't happen until the imgRequestStatusTracker's runnable executes.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 446595 [details] [diff] [review]
step 5: delay all notifications, and asynchronously send them later

Sending stuff sync from OnStartRequest seems fine, but comment 21 and comment 22 are still worrying me.

Comment 24

[Joe Drew (not getting mail)]

I will get this done this week. I've fixed some try failures, but others still remain, and I need to write tests that find the bugs I've found.

Comment 25

[Joe Drew (not getting mail)]

Attachment: step 5: delay all notifications, and asynchronously send them later

This patch fixes two bugs in the previous patch: it corrects cloning of proxies, so if we're currently in the process of loading an imgRequest we track its state changes; and it doesn't further delay notifications if we're currently loading off the network, ensuring that we get the OnStopRequest for the image before we get OnStopRequest for the channel. Both of these caused test failures.

Unfortunately, we still have a couple of test failures. The one that worries me is layout/style/test_visited_image_loading(_empty).html. It seems that the test works by waiting for the style change to be applied asynchronously, then waiting for a further draw to happen to check whether the image has been loaded. This feels wrong, since just waiting for MozAfterPaint isn't a guarantee that all the events for the image have been processed.

Is this a bad, breaking change? Or is the test just broken?

Comment 26

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

(In reply to comment #25)
> Unfortunately, we still have a couple of test failures. The one that worries me
> is layout/style/test_visited_image_loading(_empty).html. It seems that the test
> works by waiting for the style change to be applied asynchronously, then
> waiting for a further draw to happen to check whether the image has been
> loaded. This feels wrong, since just waiting for MozAfterPaint isn't a
> guarantee that all the events for the image have been processed.
> 
> Is this a bad, breaking change? Or is the test just broken?

Yeah, I agree the test is broken.

Probably the most straightforward solution to fix the test is something a little ugly like:
 * poll (starting from the paint event) every 10ms until it's in the passing state (two images loaded)
 * check how much time it took to get to that state (call that Tstate), and wait for max(100ms, 2 * Tstate), and then have the test assert that we're still in the passing state.

Comment 27

[Joe Drew (not getting mail)]

Attachment: step 0: hack around the visited image loading tests as dbaron described

This patch implements the changes to the tests, as David described. This patch, along with the other patches, is currently going through try.

Comment 28

[Joe Drew (not getting mail)]

Attachment: step 0.1: we (sometimes?) get assertions in these two tests - mark them up

I'm going to create a follow-on bug for this problem. (Although this might just be another manifestation of bug 527825.)

Let's tag David Baron for review, because he knows things about tables.

Comment 29

[Joe Drew (not getting mail)]

Comment on attachment 447370 [details] [diff] [review]
step 5: delay all notifications, and asynchronously send them later

The test fixes I've come up with convince me that this patch is correct, so let's get it reviewed.

Comment 30

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 444723 [details] [diff] [review]
step 3: remove imgContainerRequest

Some initial comments:
- I don't like the idea of having imgIContainer in the Proxy requests that's only used when we don't have a Request.
- I don't mind having a imgIContainer reference in the RequestProxy but I think we should use it for all RequestProxies
- The image status should also not be in the imageRequest proxy. The status is a property of the imgIContainer and not of the imgRequestProxy.
- I'm not exactly sure what we should do with the principal. It may be ok to keep it in the imgRequestProxy

Comment 31

[Joe Drew (not getting mail)]

Attachment: step 6: make loadImage and loadImageWithChannel scriptable

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].

Comment 32

[Joe Drew (not getting mail)]

Attachment: step 7: test async notification

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).

Comment 33

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

Comment on attachment 447827 [details] [diff] [review]
step 0.1: we (sometimes?) get assertions in these two tests - mark them up

Were the assertions on your machine or on tryserver?  I'm pretty sure this patch will turn the tree orange, since the tree is green right now, and giving it a 10-12 range would make 0 assertions give an UNEXPECTED-PASS.

Also, what assertion were you hitting?  If there's a bug filed on it, please include the bug number.

Comment 34

[Joe Drew (not getting mail)]

(In reply to comment #33)
> (From update of attachment 447827 [details] [diff] [review])
> Were the assertions on your machine or on tryserver?

Both.

>  I'm pretty sure this
> patch will turn the tree orange, since the tree is green right now, and giving
> it a 10-12 range would make 0 assertions give an UNEXPECTED-PASS.

Right, but this would need to be checked in as part of this patch set, which starts those assertions. I guess that it shouldn't be part 0.1 then, though.

> Also, what assertion were you hitting?  If there's a bug filed on it, please
> include the bug number.

The same assertions as in bug 527825, though I don't know if it's the same bug.

Comment 35

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

Comment on attachment 447599 [details] [diff] [review]
step 0: hack around the visited image loading tests as dbaron described

>+      response.write("setTimeout(function() {\n");
>+      response.write("var s = document.createElement('script');\n");
>+      response.write("s.src = 'visited_image_loading.sjs?result';\n");
>+      response.write("document.body.appendChild(s);");
>+      response.write("}, Math.max(100, Date.now() - start));\n");

I'd prefer Math.max(100, 2 * (Date.now() - start))

>+      response.write("} else {\n");
>+      response.write("var s = document.createElement('script');\n");
>+      response.write("s.src = 'visited_image_loading.sjs?waitforresult-internal';\n");
>+      response.write("document.body.appendChild(s);");
>+      response.write("}\n");

I think you should do this on a small (10ms?) setTimeout so it doesn't poll *too* aggressively.

r=dbaron with that

Comment 36

[Joe Drew (not getting mail)]

Attachment: step 3: remove imgContainerRequest, mark 2

This is a revision to the original step 3 patch that makes imgRequestProxy always have an mImage and mPrincipal, and makes imgRequest set them immediately once they're available. It doesn't move state tracking to imgContainer, because that'd be a bunch of mechanical work that'd just get undone in step 4 mark 2.

Comment 37

[Joe Drew (not getting mail)]

Attachment: step 4: centralize the tracking of image load/decode status, mark 2

This patch revises the original step 4 patch to make the imgRequestStateTracker be stored on the imgContainer, which simplifies imgRequestProxy's code.

Comment 38

[Joe Drew (not getting mail)]

Attachment: step 5: delay all notifications, and asynchronously send them later, mark 2

This is a simple bitrot fix to make Step 5 work after the changes in steps 3 and 4 mark 2.

Comment 39

[Joe Drew (not getting mail)]

Attachment: step 0.2: two assertions now fired in background-draw-nothing-malformed-images.html

We now get two of the same assertions in this test, rather than one:

REFTEST INFO | Loading file:///src/mozilla-central/layout/reftests/image-rect/background-draw-nothing-malformed-images.html
++DOMWINDOW == 19 (0x1848bc34) [serial = 19] [outer = 0x1f1324f0]
###!!! ASSERTION: ComputeActualCropRect() should not fail here: 'success', file /src/mozilla-central/layout/base/nsCSSRendering.cpp, line 3625
###!!! ASSERTION: ComputeActualCropRect() should not fail here: 'success', file /src/mozilla-central/layout/base/nsCSSRendering.cpp, line 3625

Comment 40

[Joe Drew (not getting mail)]

Attachment: step 0.1: mark up assertions better, mention bug

Comment 41

[Joe Drew (not getting mail)]

Attachment: step -1: #undef LoadImage all over the place

Some include changes make us pull windows.h in all over the place. This patch #undefs LoadImage in the places affected by this problem.

Comment 42

[Joe Drew (not getting mail)]

Attachment: step 0.3: fix the bug445810 test

The test for bug445810 assumed that, when you load an image and get its onload, that implies that onload has fired for _all_ such images, which is no longer the case. This patch adds a setTimeout(0), which will fix the problem, as the onload notification will be in the event queue already.

Comment 43

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

Comment on attachment 448939 [details] [diff] [review]
step 0.3: fix the bug445810 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?

Comment 44

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 448937 [details] [diff] [review]
step -1: #undef LoadImage all over the place

I'd rather we do this in a common header? Is that not possible?

Comment 45

[Joe Drew (not getting mail)]

No - windows.h gets pulled in all over the place.

Comment 46

[Jeff Walden [:Waldo]]

(In reply to comment #42)
> This patch adds a setTimeout(0), which will fix the problem, as the
> onload notification will be in the event queue already.

SimpleTest.executeSoon is potentially faster, and it gracefully degrades in non-Mozilla browsers.

Comment 47

[Joe Drew (not getting mail)]

In my try server runs, I'm basically clean, but for extension manager test failures. Specifically, the tests in /toolkit/mozapps/extensions/test/browser mostly fail, but only if we're running the full browser-chrome mochitest suite. (That is, running only that subdirectory always passes.) 

Mossop tells me he will be able to help me out with this on Monday. Luckily, it needn't impair the review process.

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1275547857.1275550446.8252.gz is an example of a log that shows the failures. I also see them every time I run the browser-chrome mochitest suite, but only on my Windows Vista VM, not on my OS X 10.6 machine.

Comment 48

[Joe Drew (not getting mail)]

Crap, forgot to CC Dave with the last comment.

Comment 49

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 448680 [details] [diff] [review]
step 5: delay all notifications, and asynchronously send them later, mark 2


>+  // XXX: It looks like the wrong load flags are being passed in...
>+  requestFlags &= 0xFFFF;
>+

What's up with this?

Comment 50

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 448678 [details] [diff] [review]
step 3: remove imgContainerRequest, mark 2

>+  // The URI of our request.
>+  nsCOMPtr<nsIURI> mURI;

Can mURI live inside of the imgContainer?

Comment 51

[Joe Drew (not getting mail)]

(In reply to comment #49)
> (From update of attachment 448680 [details] [diff] [review])
> 
> >+  // XXX: It looks like the wrong load flags are being passed in...
> >+  requestFlags &= 0xFFFF;
> >+
> 
> What's up with this?

Very old code that I just moved around, but there's similar code in necko.

Comment 52

[Joe Drew (not getting mail)]

(In reply to comment #50)
> (From update of attachment 448678 [details] [diff] [review])
> >+  // The URI of our request.
> >+  nsCOMPtr<nsIURI> mURI;
> 
> Can mURI live inside of the imgContainer?

It's possible, but it'll require a two-step initialization of the image in imgRequest (set the URI on construction, then the MIME type etc once we get it off the network). Dunno if that is better than what we've got.

Comment 53

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 448680 [details] [diff] [review]
step 5: delay all notifications, and asynchronously send them later, mark 2


>+  // The following notification functions are protected to ensure that (friend
>+  // class) imgRequestStatusTracker is the only class allowed to send us
>+  // notifications.
>+
>+  // Whether we want notifications from imgRequestStatusTracker.
>+  PRBool GetWantsNotifications() const
>+  {
>+    return mWantsNotifications;
>+  }
>+  void SetWantsNotifications(PRBool aWantsNotifications)
>+  {
>+    mWantsNotifications = aWantsNotifications;
>+  }
>+

Use GetDeferNotifications() instead of WantsNotifications()

Comment 54

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 448680 [details] [diff] [review]
step 5: delay all notifications, and asynchronously send them later, mark 2


>@@ -152,16 +153,19 @@ public:
>   static NS_METHOD WriteToContainer(nsIInputStream* in, void* closure,
>                                     const char* fromRawSegment,
>                                     PRUint32 toOffset, PRUint32 count,
>                                     PRUint32 *writeCount);
> 
>   PRUint32 GetDecodedDataSize();
>   PRUint32 GetSourceDataSize();
> 
>+  imgRequestStatusTracker& GetStatusTracker() { return mStatusTracker; }
>+  PRBool IsInitialized() const { return !!mInitialized; }
>+

We shouldn't need '!!'

> 
>     // Note that it's OK to add here even if the request is done.  If it is,
>     // it'll send a OnStopRequest() to the proxy in imgRequestProxy::Notify and
>     // the proxy will be removed from the loadgroup.
>     proxy->AddToLoadGroup();
> 
>-    proxy->NotifyListener();
>+    request->ConcreteImage()->GetStatusTracker().Notify(proxy);

We shouldn't need to use ConcreteImage() here. Also, as per our discussion, I think
proxy->NotifyObserver() is nicer to read.

The code for proxy->NotifyObserver() can be something like:
{
  /* it would be nice to notify the observer directly instead of through the proxy
     however we need to check mCanceled and do some stuff in OnStopRequest() before
     passing the notification on to the the observer */
  request->ConcreteImage()->GetStatusTracker().Notify(proxy);
}

>+  /* non-virtual imgIDecoderObserver methods */
>+  void OnStartDecode();
>+  void OnStartDecode(imgRequestProxy* aProxy);

Instead of having two OnStartDecode()s you could use RecordStartDecode() and (Notify|Send)OnDecode

Comment 55

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 448679 [details] [diff] [review]
step 4: centralize the tracking of image load/decode status, mark 2

How about we call this imgStatusTracker instead of imgRequestStatusTracker?

Comment 56

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 448465 [details] [diff] [review]
step 6: make loadImage and loadImageWithChannel scriptable

I don't like exposing more stuff to script but Joe wants to...

>@@ -96,25 +96,26 @@ interface imgILoader : nsISupports
>   /**
>    * Start the load and decode of an image.
>    * @param aChannel the channel to load the image from.  This must
>    *                 already be opened before ths method is called, and there
>    *                 must have been no OnDataAvailable calls for it yet.   
>    * @param aObserver the observer (may be null)
>    * @param cx some random data
>    * @param aListener [out]
>-   *        A listener that should receive the data. Can be null, in which
>-   *        case imagelib has found a cached image and is not interested in
>-   *        the data. The caller needs not cancel the channel in this case.
>+   *        A listener that you must send the channel's notifications and data to.
>+   *        Can be null, in which case imagelib has found a cached image and is
>+   *        not interested in the data. The caller needs not cancel the channel
>+   *        in this case.

I'm not sure this last sentence makes sense.

Comment 57

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 448937 [details] [diff] [review]
step -1: #undef LoadImage all over the place

I don't like scattering this all around, but I guess we don't have much choice.

Can you fix the comment to describe why we need to have it in multiple places and you can get rid of the "crazy things" part because it's not really crazy that windows.h defines LoadImage, that's just the API that they have.

Comment 58

[Joe Drew (not getting mail)]

All further development on this is going to be done in bug 572520, because this fix is too involved and dangerous to be applied to the other branches, and so we're going to try to describe it as a non-security bug fix and fix it on m-c only.

Comment 59

[Joe Drew (not getting mail)]

Without any proof that this actually causes problems, this is sg:want.

Comment 60

[Joe Drew (not getting mail)]

(In reply to comment #43)
> Comment on attachment 448939 [details] [diff] [review]
> step 0.3: fix the bug445810 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.

Comment 61

[Joe Drew (not getting mail)]

Bug 572520 is fixed, so this is too.