867755 - Support OnDataAvailable and OnStopRequest off main thread for raster image loading

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 1.0 Support OnDataAvailable and OnStopRequest off main thread for image loading

Support for OnDataAvailable and OnStopRequest callbacks off the main thread is being added in bug 497003. The use case in that bug is HTML5StreamParser. We would like to apply it to image loading too.

The goal is to reduce ODA event latency from dispatch on the socket thread to execution in the decoding threadpool. This is achieved by dispatching directly to the threadpool and skipping execution on the main thread. OnStartRequest will remain on the main thread: this is where the RedirectDeliveryTo a new nsIEventTarget is made. This propagates down to nsInputStreamPump in Necko code, in which a second call chain of CheckListenerChain calls is initiated. This checks each nsIStreamListener to determine if they all can accept ODA and OnStop off the main thread: e.g. a JS listener would respond no, since JS is only on the main thread. After this, OnStartRequest will continue to completion, and if the redirect was successful, the next callback (ODA) will be dispatched onto the event target.

Please see bug 497003 for the patch which adds the new API in nsIRetargetableStreamListener and nsIRetargetableRequest IDLs, along with changes to Necko code and HTML5StreamParser.
The initial patch uploaded in this bug is a work in progress and exists to demonstrate the idea (i.e. it crashes :) ). Feedback is requested regarding:
-- any reason why execution must take place on the main thread before delivery to the decode threadpool.
-- the decode threadpool - is it ok to deliver ODA and OnStop directly to this threadpool? Note that no change to the ordered callback sequence of OnStart (1), ODA (1 to many) and OnStop. The only difference is that execution of ODA and OnStop could be on another thread.
-- thread safety: it looks like the decoder is multi-threaded as is, but are there any areas in the code that should be made thread-safe?
-- any other warnings you may have.

Please change feedback requests to appropriate developers as needed.

Comment 1

[Bobby Holley (:bholley)]

Comment on attachment 744269 [details] [diff] [review]
WIP 1.0 Support OnDataAvailable and OnStopRequest off main thread for image loading

I'm no longer involved enough in Imagelib to give reasonable feedback on such a patch.

I would, however, request that you make sure to not hold any strong references to JS-implemented callbacks off-main-thread (I'm guessing you aren't, but just in case). That will cause us to fatally abort very soon, as soon as jdm fixes the last blocker to bug 773610.

Comment 2

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 1.1 Support OnDataAvailable and OnStopRequest off main thread for image loading

Updated patch:
Still crashes, but added in nsMainThreadPtr{Holder|Handle} for nsIURI in imgRequest, and NS_ProxyRelease for mRequest in imgRequest::OnStopRequest. More of those to come...

Comment 3

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 744384 [details] [diff] [review]
WIP 1.1 Support OnDataAvailable and OnStopRequest off main thread for image loading

Review of attachment 744384 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is a really good idea, and I'm excited you're working on it!

The biggest issue that jumps out at me is that synchronous notifications from imgRequestProxy aren't safe off the main thread. There are a lot of places this can happen. For example, imgRequest::OnDataAvailable may call imgRequest::Cancel, which can trigger notifications through imgStatusTracker::MaybeUnblockOnload. imgRequest::OnStopRequest also calls imgStatusTracker::OnStopRequest, both directly and indirectly, which again notifies.

There is existing support for deferring notifications, but I don't think it's well suited to this use case, especially since we still want notifications to be timely. One solution would be to add support in imgStatusTracker for delivering async notifications to the main thread, and activating that mode as part of the changes you made to imgRequest::OnStartRequest. (You'd also have to ensure that if the imgStatusTracker changes, as it can for example in imgRequest::OnDataAvailable, this gets propagated.)

::: image/src/RasterImage.cpp
@@ +2740,5 @@
>  
>  NS_IMETHODIMP
>  RasterImage::RequestDecodeCore(RequestDecodeType aDecodeType)
>  {
> +  //MOZ_ASSERT(NS_IsMainThread());

Although a lot of what happens in RequestDecodeCore is protected by locks and nothing immediately jumps out at me as definitely more unsafe than the existing code (which was racy anyway) it's possible that some concurrency bugs are hiding here.

Comment 4

[Steve Workman [:sworkman] (INACTIVE)]

Thanks for the feedback, Seth!

Re imgStatusTracker, yup - I've done a first pass of making things async, and just reached my first imgStatusTracker blocker, in OnDataAvailable. There's a MOZ_ASSERT(NS_IsMainThread()) in RasterImage::LockImage(), called by imgRequestProxy::SetHasImage in imgStatusTracker::ODA. So, your comments are very timely.

I'm a little concerned that something like an async LockImage could cause us to lose (some of) the benefit of delivering off the main thread. At least it terms of event latency. I'll see what I can do to bundle async messages, especially in cases like this (imgRequestProxy::SetHasImage()) where there's a for loop. At least the data is getting to the decode threadpool earlier.

And re the commented out MOZ_ASSERT in comment 3, indeed, concurrency is an issue, and there may be race conditions to deal with. Fun.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Heh, I see what you mean re: DiscardTracker::Remove. I think some methods of DiscardTracker just have to be made threadsafe.

It may be necessary to bundle notifications, but for simple cases like in imgRequestProxy::SetHasImage I wouldn't hesitate to change the API to take a "count" argument instead of looping.

Comment 6

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 1.2 Support OnDataAvailable and OnStopRequest off main thread for image loading

Update to previous patch; minor changes.

Comment 7

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 1.2 Dispatch imgRequestProxy notifications to the main thread

Dispatches proxy notifications to the main thread from a thread on the decode threadpool.
Note: This means timeliness will be of an async nature for non-deferred notifications.

Comment 8

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 1.2 Add a mutex for initial thread safety for DiscardTracker

Haven't looked into possible deadlock scenarios yet with adding these mutex locks - haven't come across any either - but the patch is not finished and pretty unstable ... so, not a lot to conclude about that :)

Comment 9

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 1.2 Dispatch FinishedSomeDecoding to the main thread

This is where things start to feel like ratholing.

The patch to dispatch FinishedSomeDecoding currently fails, because the decoding mutex is not being held by the main thread during the call. I could add a lock in DoFinishedSomeDecoding, but I'm concerned that there are a lot of changes to threading in these patches, and a delicate balance is being upset.

So, any thoughts on the way forward? My intention is not to rewrite how imagelib does multi-threading, but it feels like that is what is happening. Ideally, it would be great to get as much processing off the main thread as possible, but there's a lot of the code that assumes it has to work on the main thread. I could rework the patches to dispatch any code expecting to be on the main thread, but that feels like missing the point of delivering ODA to the decode threadpool, especially if there are assumptions about what has been completed by the time ODA or OnStop has finished.

Thoughts/Advice/Recommendations?

Comment 10

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: WIP 2.0 Support OnDataAvailable and OnStopRequest off main thread for image loading

Reflected more on this after uploading the patches earlier and decided to go ahead roll it back a little. So, I focused on those functions called in ODA and OnStopRequest that currently want to be called on the main thread, and made changes to dispatch them there. I want to get it working functionally first, and then revisit if performance requires it.

This still requires the DiscardTracker patch (attachment 746083 [details] [diff] [review], which currently has a deadlock), and I think the notifications patch too (attachment 746082 [details] [diff] [review]). But the FinishedSomeDecoding patch (attachment 746089 [details] [diff] [review]) shouldn't be needed, thus avoiding further rat-holing.

Push to try to check Tp5 and some unit tests:
-- https://tbpl.mozilla.org/?tree=Try&rev=5a656de37157

Comment 11

[Seth Fowler [:seth] [:s2h]]

(In reply to Steve Workman [:sworkman] from comment #10)
> Reflected more on this after uploading the patches earlier and decided to go
> ahead roll it back a little. So, I focused on those functions called in ODA
> and OnStopRequest that currently want to be called on the main thread, and
> made changes to dispatch them there. I want to get it working functionally
> first, and then revisit if performance requires it.

I'm totally behind this approach. Ideally we get to the point where it works, but to get good performance we need to be able to call Foo and Bar off main thread, and maybe find a way to defer doing Baz. Then those problems can be tackled individually, possibly even in separate bugs.

Comment 12

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 746083 [details] [diff] [review]
WIP 1.2 Add a mutex for initial thread safety for DiscardTracker

Review of attachment 746083 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Just glancing over this, though, it seems like there may be nasty interleavings involving mutation of sTimer and sDiscardableImages. You may need to guard those with the lock as well.

Comment 13

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v2.1 Support OnDataAvailable and OnStopRequest off main thread for image loading

Comment 14

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v2.1 Make DiscardTracker thread-safe

Comment 15

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v2.1 Add threadsafe imgRequest::GetURI

Comment 16

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v2.1 Dispatch imgRequestProxy notifications

Comment 17

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v2.2 Support OnDataAvailable and OnStopRequest off main thread for image loading

Updated with main thread checks for CheckListenerChain functions.

Comment 18

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 750817 [details] [diff] [review]
v2.2 Support OnDataAvailable and OnStopRequest off main thread for image loading

Review of attachment 750817 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/imgRequest.cpp
@@ +888,5 @@
>        LOG_MSG_WITH_PARAM(GetImgLog(), "imgRequest::OnDataAvailable", "content type", mContentType.get());
>  
> +      // XXX If server lied about mimetype and it's SVG, we may need to copy
> +      // the data and dispatch back to the main thread, AND tell the channel to
> +      // dispatch there in the future.

So it might be feasible to construct the SVG document OMT as well. Do you foresee issues other than running the parser OMT? If not, we may want to think about making the necessary changes to make this possible and stop special-casing SVG here.

I talked with Daniel Holbert about this and he thought it might (*might*) be doable without a great deal of work. He suggested that Henri Sivonen would know about what would be required to make the parser itself run OMT, and Boris Zbarsky would know whether there are any hidden gotchas in constructing the SVG document OMT. (The most obvious potential issue, scripting, is obviously not a problem for SVGs.)

Comment 19

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 750531 [details] [diff] [review]
v2.1 Add threadsafe imgRequest::GetURI

Review of attachment 750531 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/imgRequest.h
@@ +124,5 @@
>    // Update the cache entry size based on the image container
>    void UpdateCacheEntrySize();
>  
>    nsresult GetURI(nsIURI **aURI);
> +  nsresult GetURI(nsMainThreadPtrHandle<nsIURI>& aURI);

Presumably you will remove the other version of GetURI?

Comment 20

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 750528 [details] [diff] [review]
v2.1 Make DiscardTracker thread-safe

Review of attachment 750528 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/imgLoader.cpp
@@ +719,5 @@
>  NS_IMETHODIMP
>  imgCacheObserver::Observe(nsISupports* aSubject, const char* aTopic, const PRUnichar* aSomeData)
>  {
>    if (strcmp(aTopic, "memory-pressure") == 0) {
> +    MutexAutoLock lock(*DiscardTracker::sNodeListMutex);

Why isn't this located inside DiscardAll? Seems like you've wrapped both calls in this lock. And if you make that change, you can make sNodeListMutex private, which it seems like it should be.

Comment 21

[Seth Fowler [:seth] [:s2h]]

A quick metacomment: it'd be easier for me to give you feedback if you were creating new patches against your previous patches rather than making changes to the ones I've already looked over. Many small patches is totally fine. You can always merge them into a smaller number of final patches when you're ready for review.

Comment 22

[Seth Fowler [:seth] [:s2h]]

(In reply to Seth Fowler [:seth] from comment #18)
> He suggested that Henri Sivonen would
> know about what would be required to make the parser itself run OMT, and
> Boris Zbarsky would know whether there are any hidden gotchas in
> constructing the SVG document OMT. (The most obvious potential issue,
> scripting, is obviously not a problem for SVGs.)

Here's a snippet of an IRC conversation about this that contains some relevant bugzilla links:

17:33 dholbert: https://bugzilla.mozilla.org/show_bug.cgi?id=694165#c7 has a little bit of info about the yielding model that the SVG Image's parser uses (and how it bit us in that bug)
17:35 dholbert: When parsing has taken "too long", the parser uses NS_DispatchToCurrentThread() to post a callback to continue parsing
17:35 dholbert: which happens here: http://mxr.mozilla.org/mozilla-central/source/parser/htmlparser/src/nsParser.cpp#281
17:37 dholbert: Conveniently, that call is the only instance of the word "thread" in that file, which maybe is a good sign for its threading assumptions (or lack of threading assumptions)  :)
17:57 seth: cool! as you say, that's very promising
17:58 seth: sounds like henri intended to move the parsing OMT, which is also a good sign
17:58 dholbert: yeah
17:59 dholbert: http://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5StreamParser.cpp (which I initially was mistakenly digging in) has some assertions about IsParserThread()
18:00 dholbert: (I don't believe we use that class for SVG parsing, but it supports the general OMT parsing idea)
18:00 seth: yeah, especially since it appears to just check that the parser is being invoked consistently from the same thread, and *not* that it's on the main thread

Comment 23

[Steve Workman [:sworkman] (INACTIVE)]

Thanks Seth!

Re: constructing the SVG document OMT as well, let's put that in a separate bug and get raster images working first. I'll do that today.

(In reply to Seth Fowler [:seth] from comment #19)
> Comment on attachment 750531 [details] [diff] [review]
> v2.1 Add threadsafe imgRequest::GetURI
> 
> Review of attachment 750531 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/src/imgRequest.h
> @@ +124,5 @@
> >    // Update the cache entry size based on the image container
> >    void UpdateCacheEntrySize();
> >  
> >    nsresult GetURI(nsIURI **aURI);
> > +  nsresult GetURI(nsMainThreadPtrHandle<nsIURI>& aURI);
> 
> Presumably you will remove the other version of GetURI?

Let me verify that no-one is using it. I don't necessarily want to increase the number of occurrences of nsMainThreadPtrHandle unless it's really needed. But I understand wanting to keep the API simple.

(In reply to Seth Fowler [:seth] from comment #20)
> Comment on attachment 750528 [details] [diff] [review]
> v2.1 Make DiscardTracker thread-safe
> 
> Review of attachment 750528 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/src/imgLoader.cpp
> @@ +719,5 @@
> >  NS_IMETHODIMP
> >  imgCacheObserver::Observe(nsISupports* aSubject, const char* aTopic, const PRUnichar* aSomeData)
> >  {
> >    if (strcmp(aTopic, "memory-pressure") == 0) {
> > +    MutexAutoLock lock(*DiscardTracker::sNodeListMutex);
> 
> Why isn't this located inside DiscardAll? Seems like you've wrapped both
> calls in this lock. And if you make that change, you can make sNodeListMutex
> private, which it seems like it should be.

Right - I think I moved this for a specific reason, but it looks like that reason went away with a code change :)

(In reply to Seth Fowler [:seth] from comment #21)
> A quick metacomment: it'd be easier for me to give you feedback if you were
> creating new patches against your previous patches rather than making
> changes to the ones I've already looked over. Many small patches is totally
> fine. You can always merge them into a smaller number of final patches when
> you're ready for review.

So, I'm actually looking for reviews at this stage, not just feedback :) But in terms of smaller patches, bugzilla can do interdiffs for you - is that ok, or do you still want me to upload smaller patches?

Comment 24

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v2.2 Add threadsafe imgRequest::GetURI

Attended to Seth's comments re DiscardTracker's mutex.

Comment 25

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Steve Workman [:sworkman] from comment #24)
> Created attachment 751898 [details] [diff] [review]
> v2.2 Add threadsafe imgRequest::GetURI
> 
> Attended to Seth's comments re DiscardTracker's mutex.

Sigh - wrong comment - it should have said removed old GetURI as it's no longer needed.

Comment 26

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v2.2 Make DiscardTracker thread-safe

DiscardTracker mutex fixed per Seth's comment.

Comment 27

[Seth Fowler [:seth] [:s2h]]

Oh, sorry - I didn't realize the work was complete now. Would you mind posting a full try run for the current patch stack? I'll rereview once I see how try looks.

Comment 28

[Steve Workman [:sworkman] (INACTIVE)]

No worries. I wasn't clear enough. Try has been looking good. Here's a run I pushed today:
https://tbpl.mozilla.org/?tree=Try&rev=1bec8df55b67

I don't think any of the failures are caused by these patches. Let me know if you spot something.
And Tp5 looks comparable, right?

Comment 29

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 751900 [details] [diff] [review]
v2.2 Make DiscardTracker thread-safe

Review of attachment 751900 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/DiscardTracker.cpp
@@ +22,5 @@
>  /* static */ int64_t DiscardTracker::sCurrentDecodedImageBytes = 0;
>  /* static */ uint32_t DiscardTracker::sMinDiscardTimeoutMs = 10000;
>  /* static */ uint32_t DiscardTracker::sMaxDecodedImageKB = 42 * 1024;
>  /* static */ PRLock * DiscardTracker::sAllocationLock = NULL;
> +/* static */ mozilla::Mutex* DiscardTracker::sNodeListMutex;

Please explicitly initialize to nullptr. (Technically redundant, true, but still it's good style.)

@@ +170,5 @@
>    sAllocationLock = PR_NewLock();
>  
> +  // Create a lock for the node list.
> +  if (!sNodeListMutex)
> +    sNodeListMutex = new Mutex("image::DiscardTracker");

We don't expect Initialize to be called more than once, so this shouldn't be an |if|, it should be a MOZ_ASSERT that sNodeListMutex is null.

Comment 30

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 750532 [details] [diff] [review]
v2.1 Dispatch imgRequestProxy notifications

Review of attachment 750532 [details] [diff] [review]:
-----------------------------------------------------------------

Are you sure that e.g. imgStatusTracker::mImageStatus and imgStatusTracker::mState don't potentially get accessed in a racy way here? I'm having trouble convincing myself that there isn't a problem. One thing that would help, and that needs to happen before I r+ this, is that you explicitly mark in comments which imgStatusTracker methods *can* be called OMT. (You don't need to mark every leaf method - only the entry points.)

::: image/src/imgRequestProxy.cpp
@@ +95,5 @@
>    return mOwner->GetStatusTracker();
>  }
>  
> +NS_IMPL_THREADSAFE_ADDREF(imgRequestProxy)
> +NS_IMPL_THREADSAFE_RELEASE(imgRequestProxy)

Sorry if I'm missing something, but what motivates you to make this change? Presumably there's some code in imgStatusTracker that will addref imgRequestProxy OMT?

::: image/src/imgStatusTracker.cpp
@@ +37,5 @@
>  
>    virtual void OnStartDecode()
>    {
> +    // Main thread only due to mTracker->mConsumers.
> +    MOZ_ASSERT(NS_IsMainThread());

Rather than have comments for these, use the comment text as assertion text, along the lines of |MOZ_ASSERT(NS_IsMainThread(), "Must be on main thread to access mTracker->mConsumers");|.

@@ +1017,5 @@
> +    NS_DispatchToMainThread(
> +      new OnStopRequestEvent(this, aLastPart, aStatus));
> +    return;
> +  }
> +  MOZ_ASSERT(NS_IsMainThread());

I think it's OK to remove this assertion. The |if| block above is explicit enough. The same goes for OnDataAvailable.

::: image/src/imgStatusTracker.h
@@ +118,1 @@
>    void AdoptConsumers(imgStatusTracker* aTracker) { mConsumers = aTracker->mConsumers; }

If this function will cause problems, is this code ready to check in? Have you handled all the issues? If so, change this comment to note the danger and explain how to avoid it. (This may be as simple as "AdoptConsumers may only be called on the main thread".)

Comment 31

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 750817 [details] [diff] [review]
v2.2 Support OnDataAvailable and OnStopRequest off main thread for image loading

Review of attachment 750817 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/ImageFactory.cpp
@@ +93,5 @@
>                            bool aIsMultiPart,
>                            uint32_t aInnerWindowId)
>  {
> +  // Pref observers should have been initialized already.
> +  MOZ_ASSERT(gInitializedPrefCaches);

Please turn the comment here into an assertion string.

::: image/src/ImageFactory.h
@@ +26,5 @@
>    /**
> +   * Registers vars with Preferences. Should only be called on the main thread.
> +   */
> +  static void Initialize();
> +  

Looks like you have bad whitespace here, unless splinter is lying to me.

::: image/src/imgLoader.cpp
@@ +2138,5 @@
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Should be on the main thread!");
> +  nsresult rv = NS_OK;
> +  nsCOMPtr<nsIThreadRetargetableStreamListener> retargetableListener =
> +  do_QueryInterface(mDestListener, &rv);

Please indent do_QueryInterface to make it clear that this a continuation of the statement on the previous line.

::: image/src/imgRequest.cpp
@@ +727,5 @@
>    // trigger a failure, since the image might be waiting for more non-optional
>    // data and this is the point where we break the news that it's not coming.
>    if (mImage) {
> +    if (NS_IsMainThread()) {
> +      nsresult rv = mImage->OnImageDataComplete(aRequest, ctxt, status, lastPart);

Please invert the condition here so you have an early return if needed and then the rest of the code can live outside of any else block and can read in a more straight-line manner.

Also, in this method you're dispatching two events to the main thread in a very short period of time, and it doesn't seem like there's much interesting work between them. I'd suggest that it might make more sense to break this method into _two_ events, not three. The first event would subsume both ReleaseOnMainThreadEvent and OnImageDataCompleteEvent, as well as the remaining code here, and would execute on the main thread. ContinueOnStopRequest then may execute on the main thread or not depending on whether we've chosen to redirect things.

This seems like it'll result in fewer events and fewer moving parts, but it's very possible that I'm missing something important here, so let me know whether it makes sense.

@@ +794,5 @@
> +NS_IMETHODIMP
> +imgRequest::CheckListenerChain()
> +{
> +  // TODO Might need more checking here.
> +  NS_ASSERTION(NS_IsMainThread(), "Should be on the main thread!");

What's the status of this TODO?

@@ +887,5 @@
>  
>        LOG_MSG_WITH_PARAM(GetImgLog(), "imgRequest::OnDataAvailable", "content type", mContentType.get());
>  
> +      // XXX If server lied about mimetype and it's SVG, we may need to copy
> +      // the data and dispatch back to the main thread, AND tell the channel to

You say here we "may need" to copy the data and dispatch back to the main thread. What's the status of this? Is this problem resolved, and if so, how?

@@ +942,5 @@
> +    , mChan(aChan)
> +  { }
> +  NS_IMETHOD Run()
> +  {
> +    if (mImgRequest) {

mImgRequest gets |this| in SetProperties. If it's null, we're really in trouble. This should be a MOZ_ASSERT in the constructor if anything.

@@ +952,5 @@
> +  nsRefPtr<imgRequest> mImgRequest;
> +  nsCOMPtr<nsIChannel> mChan;
> +};
> +    
> +void

Splinter shows bad whitespace here.

Comment 32

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v2.3 Support OnDataAvailable and OnStopRequest off main thread for raster image loading

(In reply to Seth Fowler [:seth] from comment #31)
> Comment on attachment 750817 [details] [diff] [review]
> v2.2 Support OnDataAvailable and OnStopRequest off main thread for image
> loading
> 
> Review of attachment 750817 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/src/ImageFactory.cpp
> @@ +93,5 @@
> >                            bool aIsMultiPart,
> >                            uint32_t aInnerWindowId)
> >  {
> > +  // Pref observers should have been initialized already.
> > +  MOZ_ASSERT(gInitializedPrefCaches);
> 
> Please turn the comment here into an assertion string.

Done.

> ::: image/src/ImageFactory.h
> @@ +26,5 @@
> >    /**
> > +   * Registers vars with Preferences. Should only be called on the main thread.
> > +   */
> > +  static void Initialize();
> > +  
> 
> Looks like you have bad whitespace here, unless splinter is lying to me.

Ugh - sorry.

> ::: image/src/imgLoader.cpp
> @@ +2138,5 @@
> > +{
> > +  NS_ASSERTION(NS_IsMainThread(), "Should be on the main thread!");
> > +  nsresult rv = NS_OK;
> > +  nsCOMPtr<nsIThreadRetargetableStreamListener> retargetableListener =
> > +  do_QueryInterface(mDestListener, &rv);
> 
> Please indent do_QueryInterface to make it clear that this a continuation of
> the statement on the previous line.

Oops. Over-zealous auto-indent.

> ::: image/src/imgRequest.cpp
> @@ +727,5 @@
> >    // trigger a failure, since the image might be waiting for more non-optional
> >    // data and this is the point where we break the news that it's not coming.
> >    if (mImage) {
> > +    if (NS_IsMainThread()) {
> > +      nsresult rv = mImage->OnImageDataComplete(aRequest, ctxt, status, lastPart);
> 
> Please invert the condition here so you have an early return if needed and
> then the rest of the code can live outside of any else block and can read in
> a more straight-line manner.
> 
> Also, in this method you're dispatching two events to the main thread in a
> very short period of time, and it doesn't seem like there's much interesting
> work between them. I'd suggest that it might make more sense to break this
> method into _two_ events, not three. The first event would subsume both
> ReleaseOnMainThreadEvent and OnImageDataCompleteEvent, as well as the
> remaining code here, and would execute on the main thread.
> ContinueOnStopRequest then may execute on the main thread or not depending
> on whether we've chosen to redirect things.
> 
> This seems like it'll result in fewer events and fewer moving parts, but
> it's very possible that I'm missing something important here, so let me know
> whether it makes sense.

Agreed. It basically means OnStopRequest is being punted back to the main thread. But that should be ok, right? The data has been delivered off main thread by this stage already.

> @@ +794,5 @@
> > +NS_IMETHODIMP
> > +imgRequest::CheckListenerChain()
> > +{
> > +  // TODO Might need more checking here.
> > +  NS_ASSERTION(NS_IsMainThread(), "Should be on the main thread!");
> 
> What's the status of this TODO?

Oh - done. No more checking needed.

> @@ +887,5 @@
> >  
> >        LOG_MSG_WITH_PARAM(GetImgLog(), "imgRequest::OnDataAvailable", "content type", mContentType.get());
> >  
> > +      // XXX If server lied about mimetype and it's SVG, we may need to copy
> > +      // the data and dispatch back to the main thread, AND tell the channel to
> 
> You say here we "may need" to copy the data and dispatch back to the main
> thread. What's the status of this? Is this problem resolved, and if so, how?

Ah - I had forgotten about that, sorry. Let me come up with a solution in a follow up patch. I'll not land anything until it's ready, but maybe we can get this patch ok'd and concentrate on that separately.

> @@ +942,5 @@
> > +    , mChan(aChan)
> > +  { }
> > +  NS_IMETHOD Run()
> > +  {
> > +    if (mImgRequest) {
> 
> mImgRequest gets |this| in SetProperties. If it's null, we're really in
> trouble. This should be a MOZ_ASSERT in the constructor if anything.

Done. I converted a couple of the other assertions in runnables to MOZ_ASSERTs too.

> @@ +952,5 @@
> > +  nsRefPtr<imgRequest> mImgRequest;
> > +  nsCOMPtr<nsIChannel> mChan;
> > +};
> > +    
> > +void
> 
> Splinter shows bad whitespace here.

Ugh sorry.

Comment 33

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v2.2 Make DiscardTracker thread-safe

Thanks for the review!

(In reply to Seth Fowler [:seth] from comment #29)
> Comment on attachment 751900 [details] [diff] [review]
> v2.2 Make DiscardTracker thread-safe
> 
> Review of attachment 751900 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/src/DiscardTracker.cpp
> @@ +22,5 @@
> > +/* static */ mozilla::Mutex* DiscardTracker::sNodeListMutex;
> 
> Please explicitly initialize to nullptr. (Technically redundant, true, but
> still it's good style.)

Done.

> @@ +170,5 @@
> > +  if (!sNodeListMutex)
> > +    sNodeListMutex = new Mutex("image::DiscardTracker");
> 
> We don't expect Initialize to be called more than once, so this shouldn't be
> an |if|, it should be a MOZ_ASSERT that sNodeListMutex is null.

Done.

Comment 34

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v2.2 Dispatch imgRequestProxy notifications

(In reply to Seth Fowler [:seth] from comment #30)
> Comment on attachment 750532 [details] [diff] [review]
> v2.1 Dispatch imgRequestProxy notifications
> 
> Review of attachment 750532 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Are you sure that e.g. imgStatusTracker::mImageStatus and
> imgStatusTracker::mState don't potentially get accessed in a racy way here?
> I'm having trouble convincing myself that there isn't a problem. One thing
> that would help, and that needs to happen before I r+ this, is that you
> explicitly mark in comments which imgStatusTracker methods *can* be called
> OMT. (You don't need to mark every leaf method - only the entry points.)

So, I thought about this in terms of the changes to the current multithreading model. Currently it's OnStart, OnData and OnStop all on the main thread, all in series, with other processing happening on a decode thread. The changes are focused on OnData and OnStop, moving them off the main thread. There's no change in the sequence of OnStart etc. The change is to do with the dispatches that happen during OnData and OnStop - this potentially puts function calls inside these out of the current sequence. However, the events dispatched should be on the main thread event queue in the same order, and since they have the same priority, they should be executed in that order too. Correct me if there's an event dequeuing behavior that I'm missing.

imgStatusTracker::OnDataAvailable looks to be the only one that needs considered. OnStop has a dispatch too, but ContinueOnStopRequest continues after the dispatch on the main thread.

I did add some comments to imgRequestProxy.h for most of the functions to specify which ones should be on the main thread. And I added some more main thread checks. But most importantly, I added a comment before the dispatch in imgStatusTracker::ODA to say things should be done in order. Please check the comment and see if it makes sense.

I didn't mention mState or mImageStatus because they didn't come up within the scope of changes proposed in this bug. Nonethless, there's a lot of complexity in the decode thread pool and I can see why you'd be concerned about possible race conditions. However, I don't think dealing with more general race conditions can or should be dealt with in this bug.

Let me know if you think there's another aspect of these changes that might affect mState, mImageStatus or other variables.

> ::: image/src/imgRequestProxy.cpp
> @@ +95,5 @@
> >    return mOwner->GetStatusTracker();
> >  }
> >  
> > +NS_IMPL_THREADSAFE_ADDREF(imgRequestProxy)
> > +NS_IMPL_THREADSAFE_RELEASE(imgRequestProxy)
> 
> Sorry if I'm missing something, but what motivates you to make this change?
> Presumably there's some code in imgStatusTracker that will addref
> imgRequestProxy OMT?

You know, I swear I needed that at some stage - probably an artifact from an earlier version.

> ::: image/src/imgStatusTracker.cpp
> @@ +37,5 @@
> >  
> >    virtual void OnStartDecode()
> >    {
> > +    // Main thread only due to mTracker->mConsumers.
> > +    MOZ_ASSERT(NS_IsMainThread());
> 
> Rather than have comments for these, use the comment text as assertion text,
> along the lines of |MOZ_ASSERT(NS_IsMainThread(), "Must be on main thread to
> access mTracker->mConsumers");|.

Good idea - Done.

> @@ +1017,5 @@
> > +    NS_DispatchToMainThread(
> > +      new OnStopRequestEvent(this, aLastPart, aStatus));
> > +    return;
> > +  }
> > +  MOZ_ASSERT(NS_IsMainThread());
> 
> I think it's OK to remove this assertion. The |if| block above is explicit
> enough. The same goes for OnDataAvailable.

Yeah - I think I was over-happy with placing MOZ_ASSERTs.

> ::: image/src/imgStatusTracker.h
> @@ +118,1 @@
> >    void AdoptConsumers(imgStatusTracker* aTracker) { mConsumers = aTracker->mConsumers; }
> 
> If this function will cause problems, is this code ready to check in? Have
> you handled all the issues? If so, change this comment to note the danger
> and explain how to avoid it. (This may be as simple as "AdoptConsumers may
> only be called on the main thread".)

Oh - good catch. I updated the header file too.

Comment 35

[Steve Workman [:sworkman] (INACTIVE)]

> > @@ +887,5 @@
> > >  
> > >        LOG_MSG_WITH_PARAM(GetImgLog(), "imgRequest::OnDataAvailable", 
> > > "content type", mContentType.get());
> > >  
> > > +      // XXX If server lied about mimetype and it's SVG, we may need to copy
> > > +      // the data and dispatch back to the main thread, AND tell the channel 
> > > to
> > 
> > You say here we "may need" to copy the data and dispatch back to the main
> > thread. What's the status of this? Is this problem resolved, and if so, how?

> Ah - I had forgotten about that, sorry. Let me come up with a solution in a 
> follow up patch. I'll not land anything until it's ready, but maybe we can get 
> this patch ok'd and concentrate on that separately.

So, I'm not sure we can do anything about this. I have a patch worked up, building, running fine etc.: it's a mochitest with an html file requesting a .sjs as an image type; the .sjs returns an SVG file, but says the Content-Type is image/png. Unfortunately, sniff_mimetype_callback, or more specifically imgLoader::GetMimeTypeFromContent, can't detect SVG files. So, imgRequest::OnDataAvailable reverts to getting the mimetype from the channel, which reads from the HTTP response header and returns with image/png. So, unless GetMimeTypeFromContent is better able to detect SVG files, I don't think we can recover from this.

Current code doesn't recover from it anyway - if the server lies about an SVG file, ImageFactory::CreateImage will try to create a RasterImage anyway, right? And that's not going to work.

What do you think? If we can't detect SVG, then I don't know if this code is necessary. Maybe a comment and open a new bug (or find an existing one), and upload the test patch as an example?

Comment 36

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 756216 [details] [diff] [review]
v2.3 Support OnDataAvailable and OnStopRequest off main thread for raster image loading

Review of attachment 756216 [details] [diff] [review]:
-----------------------------------------------------------------

Lookin' good!

::: image/src/imgRequest.cpp
@@ +727,5 @@
> +/** nsThreadRetargetableStreamListener methods **/
> +NS_IMETHODIMP
> +imgRequest::CheckListenerChain()
> +{
> +  // TODO Might need more checking here.

Sounds like this comment is no longer needed.

Comment 37

[Seth Fowler [:seth] [:s2h]]

(In reply to Steve Workman [:sworkman] from comment #34)
> I didn't mention mState or mImageStatus because they didn't come up within
> the scope of changes proposed in this bug. Nonethless, there's a lot of
> complexity in the decode thread pool and I can see why you'd be concerned
> about possible race conditions. However, I don't think dealing with more
> general race conditions can or should be dealt with in this bug.

Don't worry about fixing any preexisting problems - though if you happen to do so, I won't complain! =) I mainly just want to make a reasonable effort to prevent new problems from getting introduced. As I'm sure you know figuring out whether these sorts of problems exist just by reviewing code in splinter is tricky, so I try to be a bit paranoid to make sure we're both at least thinking about the issues.

Comment 38

[Seth Fowler [:seth] [:s2h]]

(In reply to Steve Workman [:sworkman] from comment #35)
> What do you think? If we can't detect SVG, then I don't know if this code is
> necessary. Maybe a comment and open a new bug (or find an existing one), and
> upload the test patch as an example?

If the current code fails too, then it's no problem, but we should note it somewhere in bugzilla, as you say. Now that you mention it, I know I've seen a bug somewhere about mimetype sniffing not working properly for SVGs (I believe it was causing us cache-related problems). I'll try to dig it up and link it to this bug.

Comment 39

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 756218 [details] [diff] [review]
v2.2 Dispatch imgRequestProxy notifications

Review of attachment 756218 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 40

[Seth Fowler [:seth] [:s2h]]

So the SVG mimetype bug I was thinking of was bug 366324 comment 14. Not quite the right thing to link to this bug, alas.

Comment 41

[Seth Fowler [:seth] [:s2h]]

I created bug 878922 for fixing the SVG mimetype sniffing issue.

Comment 42

[Steve Workman [:sworkman] (INACTIVE)]

Awesome - thanks for the r+s and for creating the new bug.

(In reply to Seth Fowler [:seth] from comment #36)
> Comment on attachment 756216 [details] [diff] [review]
> v2.3 Support OnDataAvailable and OnStopRequest off main thread for raster
> image loading
> 
> Review of attachment 756216 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Lookin' good!
> 
> ::: image/src/imgRequest.cpp
> @@ +727,5 @@
> > +  // TODO Might need more checking here.
> 
> Sounds like this comment is no longer needed.

Indeed - I'll remove it.

FYI, this is dependent on bug 497003. I'm waiting on a final review for that one.

Comment 43

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Use DiscardTracker's mutex as a shutdown detector

Need to be able to detect if DiscardTracker is shutdown or not. Patch uses the mutex to detect this.

Comment 44

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Return already_AddRefed from GetStatusTracker instead of C++ ref

Changes GetStatusTracker to return an already_AddRefed ptr instead of a C++ ref. This is to avoid cases where the ref is cached after deletion.

Comment 45

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Add strong refcnting for usage of mozilla::image::Image objects

Increase refcnting for mozilla::image::Image objects. Changes usage to avoid NS_ADDREF and NS_RELEASE; uses nsRefPtrs and ::forget()/swap() instead.

Weak (C++) ptr kept in imgStatusTracker since Image owns the status tracker and using nsRefPtr would create a circular reference. To make this a little better, added imgStatusTracker::ResetImage to nullify the Image ptr. If something other than Image still holds a ref to imgStatusTracker after Image is deleted, this should stop it from trying to access the image.

Also, changed ImageResource and base class constructors to NOT initialize their relationship with the imgStatusTracker until AFTER the member vars are initialized. This means adding ImageResource::InitStatusTracker, using code from the constructor, and calling InitStatusTracker in RasterImage and VectorImage's constructor. This is to prevent the race condition where imgStatusTracker::SyncNotify races the constructor: there are cases where imgStatusTracker tries to call mImage->FrameRect; sometimes it thinks mImage is an ImageResource and crashes because FrameRect is a pure virtual function; other times it knows mImage is a RasterImage, but calls FrameRect before the constructor finishes, resulting in use of unintialized member vars and crashes. (2 months of try jobs to find this - yay!).

Comment 46

[Steve Workman [:sworkman] (INACTIVE)]

Full try job:
https://tbpl.mozilla.org/?tree=Try&rev=aec6e921cc7c

Comment 47

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v2.4 Support OnDataAvailable and OnStopRequest off main thread for raster image loading

Seth: Updated repo and pulled in the nsMainThreadPtrHandle/Holder changes (Bug 912514 - Don't release nsIURIs off-main-thread). I had also changed the URIs to use nsMainThreadPtrHandler/Holder, but back up a step in imgRequest.cpp. Also, adding ODA off main thread support requires off main thread URI access, so I removed the boolean flag you had linked to DEBUG_imagelib. Let me know what you think here.

Comment 48

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 804642 [details] [diff] [review]
v1.0 Use DiscardTracker's mutex as a shutdown detector

Review of attachment 804642 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/DiscardTracker.cpp
@@ +84,5 @@
>  {
> +  if (!sNodeListMutex) {
> +    // Already shutdown. List should be empty, so just return.
> +    return;
> +  }

Is this safe? It concerns me that the process of shutting down may be almost totally complete - we may even have already deleted sNodeListMutex - and yet we may read a non-null value for sNodeListMutex here. To make this safe it'd be better to either take the lock or set a flag atomically in Shutdown.

Comment 49

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 804648 [details] [diff] [review]
v1.0 Return already_AddRefed from GetStatusTracker instead of C++ ref

Review of attachment 804648 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/RasterImage.cpp
@@ +1084,5 @@
>      // Notify our observers that we are starting animation.
> +    nsRefPtr<imgStatusTracker> statusTracker = CurrentStatusTracker();
> +    if (statusTracker) {
> +      statusTracker->RecordImageIsAnimated();
> +    }

Can we actually get a null value back from CurrentStatusTracker? Clearly in the past we assumed that that wasn't possible. If that hasn't changed then the 'if' here is not needed, but what might be appropriate instead is an assert in CurrentStatusTracker that we're returning a non-null value.

@@ +1737,2 @@
>      mStatusTracker->GetDecoderObserver()->OnStopRequest(aLastPart, finalStatus);
>    }

Probably this chunk of code can be neater and less repetitive by rewriting it to use CurrentStatusTracker.

@@ +2858,2 @@
>      mStatusTracker->GetDecoderObserver()->OnError();
>    }

This chunk of code should also be rewritten to use CurrentStatusTracker.

::: image/src/RasterImage.h
@@ +319,1 @@
>      }

You don't have to do this, but if it was me I'd initialize statusTracker with the ternary operator here.

@@ +336,5 @@
>        , mAllocatedNewFrame(false)
>      {
> +      MOZ_ASSERT(aImage, "aImage cannot be null");
> +      MOZ_ASSERT(aImage->mStatusTracker,
> +                 "aImage should gave an imgStatusTracker");

"gave" -> "have"?

::: image/src/imgRequestProxy.cpp
@@ +71,5 @@
>  {
>    if (!mOwnerHasImage)
>      return nullptr;
> +  nsRefPtr<imgStatusTracker> statusTracker = GetStatusTracker();
> +  return statusTracker ? statusTracker->GetImage() : nullptr;

Re: this and all the other places in this file where we check the return value of GetStatusTracker: presumably now this can happen? If not we can probably skip the checks and just put an assert in GetStatusTracker itself.

@@ +935,5 @@
>    // instead of through the proxy, but there are several places we do extra
>    // processing when we receive notifications (like OnStopRequest()), and we
>    // need to check mCanceled everywhere too.
>  
> +  nsRefPtr<imgStatusTracker> statusTracker = GetStatusTracker();

I notice there's no null-check for statusTracker in this case. If it can be null sometimes now, is this intentional?

::: image/src/imgStatusTracker.cpp
@@ +23,5 @@
>  public:
>    imgStatusTrackerNotifyingObserver(imgStatusTracker* aTracker)
> +  : mTracker(aTracker) {
> +    MOZ_ASSERT(aTracker);
> +  }

Nit: This formatting seems inconsistent with local style to me.

@@ +215,5 @@
>  public:
>    imgStatusTrackerObserver(imgStatusTracker* aTracker)
> +  : mTracker(aTracker->asWeakPtr()) {
> +    MOZ_ASSERT(aTracker);
> +  }

Same here.

@@ +473,5 @@
>      NS_IMETHOD Run()
>      {
>        MOZ_ASSERT(NS_IsMainThread(), "Should be running on the main thread");
>        MOZ_ASSERT(mProxy, "mProxy cannot be null");
> +      MOZ_ASSERT(mStatusTracker, "mStatusTracker cannot be null");

Do we need to assert about mProxy and mStatusTracker in both the constructor and here? Can they change once set? (I realize the mProxy assert was here already. =)

@@ +680,5 @@
>  {
> +  // Grab a ref to this to ensure it isn't deleted.
> +  nsRefPtr<imgStatusTracker> thisStatusTracker = this;
> +  nsRefPtr<imgStatusTracker> clone = new imgStatusTracker(*thisStatusTracker);
> +  return clone.forget();

Could anything actually go wrong here? imgStatusTracker's constructor isn't side effecting, is it?

Comment 50

[Seth Fowler [:seth] [:s2h]]

(In reply to Steve Workman [:sworkman] from comment #47)
> Also, adding ODA off main thread support requires off main
> thread URI access, so I removed the boolean flag you had linked to
> DEBUG_imagelib. Let me know what you think here.

AFAIK this isn't safe because nsStandardURL's are not threadsafe internally, even though it'll probably work right 99.99% of the time. I don't think it's safe to just remove that flag, but we can do something more safe that would achieve the same result - for example, we can store a string instead of an nsIURI, which also eliminates the need for nsMainThreadPtrHolder/Handle.

Comment 51

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Add imgURLData to store threadsafe URL data

Seth, I've added a new class to Image.h called imgURLData to get around nsStandardURL. This patch should be applied on top of the core patch (attachment 756216 [details] [diff] [review]) - for now, this is so you can let me know what you think about these specific changes, but I'll roll it up into one patch when it's ready.

For now, this new class is for imagelib code only. Let's see what it might grow into, or what other needs it might satisfy. But Image.h for now.

It doesn't implement the nsIURI interface, because I want to keep it walled in to imagelib code only. If it implements nsIURI, there's a chance it could 'leak' out somewhere undetected; I think it's better to be clear about what URL class is being used in this case, at least for now - if it develops into something bigger over time, sure. But for now, let's keep a minimal implementation.

That means, however, that there is some function overloading where required. E.g. imgLoader has several functions that are implemented for nsIURI* and imgURLData. I've tried to add common functions in those cases, to reduce some redundancy.

Also, it has three string fields, which means a bit of redundancy in memory, but I'm trying to avoid redundancy in the code, and also complexity. Instead of implementing functions to parse the URL, I've written it to rely on the implementation behind the nsIURI pointer that initializes it. Adding that parsing, or sharing it with nsStandardURL can be future bugs if needed.

Let me know what you think.

Comment 52

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.1 Return already_AddRefed from GetStatusTracker instead of C++ ref

Thanks Seth!

(In reply to Seth Fowler [:seth] from comment #49)
> Comment on attachment 804648 [details] [diff] [review]
> v1.0 Return already_AddRefed from GetStatusTracker instead of C++ ref
> 
> Review of attachment 804648 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/src/RasterImage.cpp
> @@ +1084,5 @@
> >      // Notify our observers that we are starting animation.
> > +    nsRefPtr<imgStatusTracker> statusTracker = CurrentStatusTracker();
> > +    if (statusTracker) {
> > +      statusTracker->RecordImageIsAnimated();
> > +    }
> 
> Can we actually get a null value back from CurrentStatusTracker? Clearly in
> the past we assumed that that wasn't possible. If that hasn't changed then
> the 'if' here is not needed, but what might be appropriate instead is an
> assert in CurrentStatusTracker that we're returning a non-null value.

Yeah, this should be better. I've changed CurrentStatusTracker to have a MOZ_ASSERT, and removed the null check from the call.

> @@ +1737,2 @@
> >      mStatusTracker->GetDecoderObserver()->OnStopRequest(aLastPart, finalStatus);
> >    }
> 
> Probably this chunk of code can be neater and less repetitive by rewriting
> it to use CurrentStatusTracker.

Agreed. Done.

> @@ +2858,2 @@
> >      mStatusTracker->GetDecoderObserver()->OnError();
> >    }
> 
> This chunk of code should also be rewritten to use CurrentStatusTracker.

Done.

> ::: image/src/RasterImage.h
> @@ +319,1 @@
> >      }
> 
> You don't have to do this, but if it was me I'd initialize statusTracker
> with the ternary operator here.

Yup, looks nicer. Done.

> @@ +336,5 @@
> >        , mAllocatedNewFrame(false)
> >      {
> > +      MOZ_ASSERT(aImage, "aImage cannot be null");
> > +      MOZ_ASSERT(aImage->mStatusTracker,
> > +                 "aImage should gave an imgStatusTracker");
> 
> "gave" -> "have"?

aImage is required to be altruistic and giving is to be enforced! ;p
Typo fixed.

> ::: image/src/imgRequestProxy.cpp
> @@ +71,5 @@
> >  {
> >    if (!mOwnerHasImage)
> >      return nullptr;
> > +  nsRefPtr<imgStatusTracker> statusTracker = GetStatusTracker();
> > +  return statusTracker ? statusTracker->GetImage() : nullptr;
> 
> Re: this and all the other places in this file where we check the return
> value of GetStatusTracker: presumably now this can happen? If not we can
> probably skip the checks and just put an assert in GetStatusTracker itself.

Agreed. Assert in GetStatusTracker (all implementations that are returning a ref to a member var; not those that are calling another GetStatusTracker function).

> @@ +935,5 @@
> >    // instead of through the proxy, but there are several places we do extra
> >    // processing when we receive notifications (like OnStopRequest()), and we
> >    // need to check mCanceled everywhere too.
> >  
> > +  nsRefPtr<imgStatusTracker> statusTracker = GetStatusTracker();
> 
> I notice there's no null-check for statusTracker in this case. If it can be
> null sometimes now, is this intentional?

Indeed. Tried to make these more consistent. No null checks in code; assert in the function itself.

> ::: image/src/imgStatusTracker.cpp
> @@ +23,5 @@
> >  public:
> >    imgStatusTrackerNotifyingObserver(imgStatusTracker* aTracker)
> > +  : mTracker(aTracker) {
> > +    MOZ_ASSERT(aTracker);
> > +  }
> 
> Nit: This formatting seems inconsistent with local style to me.

Fixed.

> @@ +215,5 @@
> >  public:
> >    imgStatusTrackerObserver(imgStatusTracker* aTracker)
> > +  : mTracker(aTracker->asWeakPtr()) {
> > +    MOZ_ASSERT(aTracker);
> > +  }
> 
> Same here.

Also fixed.

> @@ +473,5 @@
> >      NS_IMETHOD Run()
> >      {
> >        MOZ_ASSERT(NS_IsMainThread(), "Should be running on the main thread");
> >        MOZ_ASSERT(mProxy, "mProxy cannot be null");
> > +      MOZ_ASSERT(mStatusTracker, "mStatusTracker cannot be null");
> 
> Do we need to assert about mProxy and mStatusTracker in both the constructor
> and here? Can they change once set? (I realize the mProxy assert was here
> already. =)

Yeah, I was being extra cautious. Asserts in constructor, none in Run.

> @@ +680,5 @@
> >  {
> > +  // Grab a ref to this to ensure it isn't deleted.
> > +  nsRefPtr<imgStatusTracker> thisStatusTracker = this;
> > +  nsRefPtr<imgStatusTracker> clone = new imgStatusTracker(*thisStatusTracker);
> > +  return clone.forget();
> 
> Could anything actually go wrong here? imgStatusTracker's constructor isn't
> side effecting, is it?

It shouldn't be. This should be a simple copy only.

Comment 53

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 805477 [details] [diff] [review]
v2.4 Support OnDataAvailable and OnStopRequest off main thread for raster image loading

Canceling the review on this one - subsequent changes are in attachment 807954 [details] [diff] [review], so better to look at that one instead.

Comment 54

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.1 Detect if DiscardTracker has shutdown

Using an Atomic<uint32_t> for a shutdown flag in DiscardTracker. (Changed from using the nullness of the mutex).

Comment 55

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 807954 [details] [diff] [review]
v1.0 Add imgURLData to store threadsafe URL data

Review of attachment 807954 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good!

If you haven't already, please file a bug for improving the memory usage of imgURLData and CC me on it. We'll eventually want to be smarter about that, as you mention.

Also, I should probably let you know that in general the trend in imagelib is to move from calling things "imgBlah" to "ImageBlah" (or just "Blah", given that we have namespaces now). Eventually we'll rename all the old-style stuff to make things consistent. I might have thus called "imgURLData" "ImageURL" or something. Given how much files this patch touches I'm fine with it if you want to keep it imgURLData, though. It's not really a big deal, like most style stuff. =)

::: image/src/Image.h
@@ +29,5 @@
> + * By not implementing nsIURI, external code cannot unintentionally be given an
> + * nsIURI pointer with this limited class behind it; instead, conversion to a
> + * fully implemented nsIURI is required (e.g. through NS_NewURI).
> + */
> +class imgURLData

Please put this in its own header file instead of in Image.h.

@@ +32,5 @@
> + */
> +class imgURLData
> +{
> +public:
> +  imgURLData(nsIURI* aURI) {

Based on looking at the rest of the code, I suspect you don't want an implicit conversion from nsIURI* to imgURLData, right? Mark this constructor explicit.

@@ +72,5 @@
> +  // This means each field is stored separately, but since only a few are
> +  // required, this small memory tradeoff for threadsafe usage should be ok.
> +  nsCString mSpec;
> +  nsCString mScheme;
> +  nsCString mRef;

Suggestion: use nsAutoCString for these to reduce the number of heap allocations we have to make.

@@ +246,5 @@
>    virtual nsresult StopAnimation() = 0;
>  
>    // Member data shared by all implementations of this abstract class
>    nsRefPtr<imgStatusTracker>    mStatusTracker;
> +  nsRefPtr<imgURLData>       mURI;

Nit: align mURI with mStatusTracker.

::: image/src/imgLoader.h
@@ +273,5 @@
>    nsresult InitCache();
>  
>    bool RemoveFromCache(nsIURI *aKey);
> +  bool RemoveFromCache(imgURLData *aKey);
> +  bool RemoveFromCache(nsCString& spec,

Nit: Although I greatly prefer "nsCString& spec" to "nsCString &spec", in the name of local style I suppose this should be the latter rather than the former.

::: image/src/imgRequest.cpp
@@ +88,5 @@
>                            nsIPrincipal* aLoadingPrincipal,
>                            int32_t aCORSMode)
>  {
> +  MOZ_ASSERT(NS_IsMainThread(), "Cannot use nsIURI off main thread!");
> +  

Whitespace.

@@ +264,5 @@
>    if (mRequest && statusTracker.IsLoading())
>      mRequest->Cancel(aStatus);
>  }
>  
>  nsresult imgRequest::GetURI(nsIURI **aURI)

This looks like something that belongs in a method on imgURLData ("ToIURI"?). Converting between imgURLData and nsIURI probably shouldn't be the responsibility of imgRequest. That change would allow you to get rid of this unsafe overload of GetURI altogether.

::: image/src/imgRequest.h
@@ +130,3 @@
>    nsresult GetURI(nsIURI **aURI);
> +  // OK to use on any thread.
> +  nsresult GetURI(imgURLData **aURI);

The distinction here seems a little too important to make in terms of overloading alone. Can we distinguish these methods by name? Maybe "GetURIUnsafe" (for nsIURI) and "GetURI" (for imgURLData)?

But actually we should probably just get rid of the nsIURI version. See my comments for the implementation of that method.

::: image/src/imgRequestProxy.cpp
@@ +511,5 @@
> +  nsCOMPtr<nsIURI> result;
> +  NS_NewURI(getter_AddRefs(result), uriSpec);
> +  result.forget(aURI);
> +  return NS_OK;
> +  //return NS_ERROR_NOT_IMPLEMENTED;

Again, probably should have "ToIURI" or similar on imgURLData.

::: image/src/imgStatusTracker.cpp
@@ +1076,5 @@
> +    nsRefPtr<imgURLData> threadsafeUriData = GetImage()->GetURI();
> +    nsAutoCString spec;
> +    threadsafeUriData->GetSpec(spec);
> +    nsCOMPtr<nsIURI> uri;
> +    NS_NewURI(getter_AddRefs(uri), spec);

Again this looks like imgURLData should have a "ToIURI" method or something similar.

Comment 56

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 804659 [details] [diff] [review]
v1.0 Add strong refcnting for usage of mozilla::image::Image objects

Review of attachment 804659 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/RasterImage.cpp
@@ +403,5 @@
>    mWantFullDecode(false),
>    mScaleRequest(nullptr)
>  {
> +  InitStatusTracker(aStatusTracker);
> +  MOZ_ASSERT(mStatusTracker);

So this pattern of calling InitStatusTracker in the constructor and ResetImage in the destructor seems very RAII to me. Consider either letting ImageResource's constructor/destructor handle this or adding a RAII wrapper class to handle this automatically.

::: image/src/VectorImage.cpp
@@ +318,5 @@
>  {
> +  // Clear weak reference to this image.
> +  MOZ_ASSERT(mStatusTracker);
> +  mStatusTracker->ResetImage();
> +  

Whitespace.

::: image/src/imgRequestProxy.cpp
@@ +505,5 @@
> +  nsRefPtr<Image> image = GetImage();
> +  nsCOMPtr<imgIContainer> imageToReturn;
> +  if (image)
> +    imageToReturn = do_QueryInterface(image);
> +  if ((!image || !imageToReturn) && GetOwner())

nsCOMPtr's are initialized to null so you can just write "!imageToReturn && GetOwner()" here if you want.

::: image/src/imgStatusTracker.cpp
@@ +75,4 @@
>                        "OnStartContainer callback before we've created our image");
> +    nsRefPtr<Image> image = mTracker->GetImage();
> +    mTracker->RecordStartContainer(image);
> +    image = nullptr;

Nit: Generally in this kind of situation I'd suggest wrapping this code in an anonymous scope instead of setting |image| to null at the end. The benefit is that then code that tries to refer to image later won't compile, and the fact that the intention is for the destructor to run at the end of the scope is very clear to the reader.

::: image/src/imgStatusTracker.h
@@ +314,5 @@
>    // frames are assumed to be fully valid.)
>    nsIntRect mInvalidRect;
>  
> +  // The image owns the status tracker and must call ResetImage in its
> +  // destructor to clear this weak ptr.

It'd be good to enforce that ResetImage is called using code rather than a comment (see my comment elsewhere about RAII). To make this comment stand out more strongly, considering uppercasing "must".

Comment 57

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 807965 [details] [diff] [review]
v1.1 Return already_AddRefed from GetStatusTracker instead of C++ ref

Review of attachment 807965 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there! This is looking very good but the WeakPtr issue mentioned below prevents me from r+'ing. =(

::: image/src/imgRequest.cpp
@@ +351,5 @@
>    // concern though is that image loads remain lower priority than other pieces
>    // of content such as link clicks, CSS, and JS.
>    //
> +  nsRefPtr<imgStatusTracker> statusTracker = GetStatusTracker();
> +  if (statusTracker->FirstConsumerIs(proxy))

Do my eyes deceive me, or is the original condition "!GetStatusTracker().FirstConsumerIs(proxy)"? The logic appears to be inverted now.

::: image/src/imgStatusTracker.cpp
@@ +324,5 @@
>      mTracker->RecordError();
>    }
>  
>  private:
> +  mozilla::WeakPtr<imgStatusTracker> mTracker;

I _really really really really_ don't like this WeakPtr API. I don't see how this can possibly be thread safe, since this API does not involve taking a strong reference to the object.

For me to r+ this patch, one of three things must be the case:

1. There's 100% for sure no way the underlying |imgStatusTracker| can be destroyed while this code is running (but then why'd you need a weak pointer?), or...

2. This gets switched over to use the XPCOM weak reference facility instead of the mfbt one, or...

3. You convince me I'm wrong. =)

Comment 58

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 808837 [details] [diff] [review]
v1.1 Detect if DiscardTracker has shutdown

Review of attachment 808837 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 59

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v2.0 Return already_AddRefed from GetStatusTracker instead of C++ ref

(In reply to Seth Fowler [:seth] from comment #57)
> ::: image/src/imgRequest.cpp
> @@ +351,5 @@
> >    // concern though is that image loads remain lower priority than other pieces
> >    // of content such as link clicks, CSS, and JS.
> >    //
> > +  nsRefPtr<imgStatusTracker> statusTracker = GetStatusTracker();
> > +  if (statusTracker->FirstConsumerIs(proxy))
> 
> Do my eyes deceive me, or is the original condition
> "!GetStatusTracker().FirstConsumerIs(proxy)"? The logic appears to be
> inverted now.

Opps. Good catch. Fixed.

> ::: image/src/imgStatusTracker.cpp
> @@ +324,5 @@
> >      mTracker->RecordError();
> >    }
> >  
> >  private:
> > +  mozilla::WeakPtr<imgStatusTracker> mTracker;
> 
> I _really really really really_ don't like this WeakPtr API. I don't see how
> this can possibly be thread safe, since this API does not involve taking a
> strong reference to the object.
> 
> For me to r+ this patch, one of three things must be the case:
> 
> 1. There's 100% for sure no way the underlying |imgStatusTracker| can be
> destroyed while this code is running (but then why'd you need a weak
> pointer?), or...
> 
> 2. This gets switched over to use the XPCOM weak reference facility instead
> of the mfbt one, or...
> 
> 3. You convince me I'm wrong. =)

So, I played around with this some more and we talked offline:
1. Alas, imgStatusTracker could be destroyed while this code is running. To mitigate the pointer from hanging in the ether, I've promoted it to a strong pointer in each function call. So, it should not go away until each function ends at least.

2. XPCOM Weak Reference is not suitable because imgStatusTracker does not implement an XPIDL interface, so do_QueryReferent cannot be used.

3. You're not wrong, but with the tools available, the time already spent on this, and the perceived low probability of it being a serious issue, we're going to stick with WeakPtr for now (this is after we discussed this, I'm not just telling you how it is ;) )

Let me know if this patch is better.

Comment 60

[Steve Workman [:sworkman] (INACTIVE)]

Try: Looks good to me: unrelated failures only.

https://tbpl.mozilla.org/?tree=Try&rev=5bc91860c025

Comment 61

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 810865 [details] [diff] [review]
v2.0 Return already_AddRefed from GetStatusTracker instead of C++ ref

Review of attachment 810865 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: image/src/imgRequest.cpp
@@ +253,5 @@
>  
> +  nsRefPtr<imgStatusTracker> statusTracker = GetStatusTracker();
> + 
> +  statusTracker->MaybeUnblockOnload();
> + 

Nit: Whitespace on lines 255 and 257.

Comment 62

[Steve Workman [:sworkman] (INACTIVE)]

Thanks Seth!

https://hg.mozilla.org/integration/mozilla-inbound/rev/799dffa9306b
https://hg.mozilla.org/integration/mozilla-inbound/rev/68378e8901f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ab541ed3c69
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d8f3119bad7
https://hg.mozilla.org/integration/mozilla-inbound/rev/22c38ee36fe9
https://hg.mozilla.org/integration/mozilla-inbound/rev/5df5c70b3002

Comment 63

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/799dffa9306b
https://hg.mozilla.org/mozilla-central/rev/68378e8901f3
https://hg.mozilla.org/mozilla-central/rev/3ab541ed3c69
https://hg.mozilla.org/mozilla-central/rev/5d8f3119bad7
https://hg.mozilla.org/mozilla-central/rev/22c38ee36fe9
https://hg.mozilla.org/mozilla-central/rev/5df5c70b3002