622840 - run moz-icon OS integration asynchronously

Status: RESOLVED DUPLICATE of bug 1530190

(Core :: Graphics: ImageLib, defect)

Description

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

From the MSDN documentation for SHGetFileInfo:

"You should call this function from a background thread. Failure to do so could cause the UI to stop responding."

Indeed, where we use a lot of -moz-icons, such as the download manager, it can be sad-times.  There doesn't seem to be a good way to do them asynchronously other than on a background thread on Windows, dunno about other platforms.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This should be pretty easy to do.  If this call happened in an input stream instead of a channel Necko could throw it on a background thread for us.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So, I lied, it's slightly trickier than that.  We have to do a bit of homework on the main thread first because some of these objects aren't threadsafe.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Here's a patch for Windows.  It's a little big, but it's mostly moving code around.  Of note:

1. We create an nsIconInputStream class to do most of the dirty work.
2. We gut nsIconChannel into some static helpers and nsIconInputStream.  What's left is basically a shell that makes input streams and handles channel stuff.
3. nsIconInputStream is a blocking input stream that does (among other things) the API calls here.  Because it's blocking, Necko shoves it on a transport thread, moving it out of the main thread's path.
4. We have to do some preliminary work in the constructor and proxy a release in the destructor because the URI isn't threadsafe.
5. I made the buffer allocation fallible because I couldn't see a reason not to.

I don't think we have much in the way of tests for this, but I played around with it for a bit and verified it works/doesn't crash/doesn't assert.

Comment 4

[Shawn Wilsher :sdwilsh]

Could you also add assertions to the methods that call the windows API to make sure they are not running on the main thread?

Comment 5

[Shawn Wilsher :sdwilsh]

Also, if you really have to use the nsIURI on the background thread, you should clone the one you get in case it changes on the main thread (which could happen).  Really though, you should avoid passing nsIURI objects to the background thread (I've learned this the hard way with Places stuff).

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Added thread assertions and cloned the URI per sdwilsh's comments.

Comment 7

[Joe Drew (not getting mail)]

Comment on attachment 501132 [details] [diff] [review]
Patch

I would also be ok with a followup (or even predecessor!) patch to remove the Windows CE goop, FYI :)

Couple of questions/comments:
 - nsIconInputStream's constructor doesn't check if do_QueryInterface fails on the URI, and it probably should. Not sure what we could do in that case though...
 - Switch out NS_ASSERTION for NS_ABORT_IF_FALSE for main thread. Assertions are too easy to ignore.
 - Why do you hand-roll NS_IMPL_THREADSAFE_ISUPPORTS for nsIconInputStream?

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #7)
> Comment on attachment 501132 [details] [diff] [review]
> Patch
> 
> I would also be ok with a followup (or even predecessor!) patch to remove the
> Windows CE goop, FYI :)

OK.

> Couple of questions/comments:
>  - nsIconInputStream's constructor doesn't check if do_QueryInterface fails on
> the URI, and it probably should. Not sure what we could do in that case
> though...

Assert/abort presumably, since that should never happen.

>  - Switch out NS_ASSERTION for NS_ABORT_IF_FALSE for main thread. Assertions
> are too easy to ignore.

Sure.

>  - Why do you hand-roll NS_IMPL_THREADSAFE_ISUPPORTS for nsIconInputStream?

No reason.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Kill WINCE icon code

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Carrying forward r+

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Proxy call into nsIMIMEService to the main thread

Unfortunately nsExternalAppHelperService is not threadsafe, and we can end up calling into it from what is now a background thread.  Hence the need for this ugly proxying code. :-/

Comment 12

[Josh Matthews [:jdm]]

Any condition variable can be spuriously notified, so you should always call Wait() in a loop.

Comment 13

[Joe Drew (not getting mail)]

Comment on attachment 520711 [details] [diff] [review]
Proxy call into nsIMIMEService to the main thread


>+class ExtensionFetcherRunnable : public nsRunnable {
>+typedef mozilla::Mutex Mutex;
>+typedef mozilla::CondVar CondVar;

Indent these?

>+  NS_IMETHOD Run() {

{ on its own line please.

>+    // If the mime service does not know about this mime type, we show
>+    // the generic icon.
>+    // In any case, we need to insert a '.' before the extension.
>+    mFilePath = NS_LITERAL_STRING(".") + NS_ConvertUTF8toUTF16(defFileExt);
>+    mCondVar.Notify();
>+    return NS_OK;

You should split the Notify out visually.

>+  void Call() {
>+    mozilla::MutexAutoLock autoLock(mLock);
>+    NS_DispatchToMainThread(this);
>+    mCondVar.Wait();

As Josh mentions, you'll (apparently) need to put the Wait in a loop until either we're sure Run() has finished, either in an error state or successfully.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/b65953071f2f
http://hg.mozilla.org/mozilla-central/rev/5bb54831a0ca
http://hg.mozilla.org/mozilla-central/rev/ce53f5dd0092

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ehsan backed this out because it caused a Win7 reftest orange :-/

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=f34c27ea9f43

Comment 17

[(no longer active)]

FWIW, I saw the orange on both debug and opt builds.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I landed the WINCE code removal parts again in http://hg.mozilla.org/mozilla-central/rev/98ef084ded47.  Still need to figure out the test failure.