330394 - LoadImage should not call nsILoadGroup::RemoveRequest

Status: RESOLVED WORKSFORME

(Core :: Graphics: ImageLib, defect, P1)

Description

[Darin Fisher]

LoadImage should not call nsILoadGroup::RemoveRequest

This results in all kinds of nastiness for the consumer of ImageLib.  See, for example, bug 330089.  We should make this call always happen asynchronously for consistency.

Comment 1

[Darin Fisher]

The call to RemoveRequest is indirect.  It is a side-effect.  See this stack trace: https://bugzilla.mozilla.org/attachment.cgi?id=214874

Comment 2

[Boris Zbarsky [:bzbarsky]]

So this may sound silly... but if we're going to synchronously do the whole shebang here like we do now, could we just not call either AddRequest or RemoveRequest?

I do think that doing it all async would be much better, though...

Comment 3

[Darin Fisher]

> So this may sound silly... but if we're going to synchronously do the whole
> shebang here like we do now, could we just not call either AddRequest or
> RemoveRequest?

Maybe... are there cases where we would miss the WPL notifications?

Comment 4

[Boris Zbarsky [:bzbarsky]]

We never send OnProgress here, right?  I guess someone who's trying to keep track of all loads might care, though....

OK, let's just make this stuff async.  ;)

Comment 5

[Darin Fisher]

Yeah, I think we should.  It would help at least one extension that I've worked on that wants to inspect imgIRequest objects as they are loaded by the browser.

Comment 6

[Darin Fisher]

Attachment: v1 patch

This is a candidate patch.  I'm still in the process of running tests on it to determine perf impact.  My theory is that it may be sufficient from a performance point of view to fire OnStartDecode and OnStartContainer synchronous, and then defer the rest of the notifications until a PLEvent runs.  This patch implements that design, and I'll report back shortly with performance results.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Fwiw, if you have time to test doing it all async, that would be great too...  I wouldn't be suprised if that has no perf hit (but I wouldn't be surprised if it has one too...)

Comment 8

[Darin Fisher]

Sure, I'll try that too.

Comment 9

[Darin Fisher]

OK, running the Tp test on my T41p thinkpad, I didn't see any markable difference in Tp results:

                 median      average
   baseline:        198          209
   v1 patch:        196          214
 full async:        198          210

Perhaps there'd be a measurable difference on a slower machine.

Comment 10

[Darin Fisher]

Comment on attachment 218240 [details] [diff] [review]
v1 patch

I'd like to go with this patch.  I think there may be some testcases out there that might benefit (even slightly) from synchronous OnStartContainer since that would avoid some layout reflow.

Comment 11

[Darin Fisher]

Comment on attachment 218240 [details] [diff] [review]
v1 patch

I think more is needed.  imgIRequest::Cancel triggers synchronous callbacks, which could result in ordering problems if those callbacks were followed by the callbacks generated from NotifyProxyListener.

Comment 12

[Darin Fisher]

-> defaults

Comment 13

[Boris Zbarsky [:bzbarsky]]

We should really fix this...

Comment 14

[Benoit Girard (:BenWa)]

Do we still need this Boris?

Comment 15

[Joe Drew (not getting mail)]

imgIRequest::Cancel is asynchronous now; we have a synchronous variant, CancelAndForgetObserver, but even it removes from the load group asynchronously.

This looks fine now.