run moz-icon OS integration asynchronously

REOPENED
Unassigned

Status

()

REOPENED
8 years ago
2 years ago

People

(Reporter: shaver, Unassigned)

Tracking

({main-thread-io, perf})

Trunk
mozilla5
main-thread-io, perf
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

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.
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.
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.
Created attachment 501084 [details] [diff] [review]
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.
Assignee: nobody → khuey
Status: NEW → ASSIGNED
Attachment #501084 - Flags: review?(joe)
Could you also add assertions to the methods that call the windows API to make sure they are not running on the main thread?
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).

Updated

8 years ago
Attachment #501084 - Flags: feedback+
Created attachment 501132 [details] [diff] [review]
Patch

Added thread assertions and cloned the URI per sdwilsh's comments.
Attachment #501084 - Attachment is obsolete: true
Attachment #501132 - Flags: review?(joe)
Attachment #501084 - Flags: review?(joe)

Updated

8 years ago
status2.0: ? → ---
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?
Attachment #501132 - Flags: review?(joe) → review+
(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.
Created attachment 520711 [details] [diff] [review]
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. :-/
Attachment #520711 - Flags: review?
Attachment #520711 - Flags: feedback?(sdwilsh)
Any condition variable can be spuriously notified, so you should always call Wait() in a loop.
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.
Attachment #520711 - Flags: review?(joe) → review+
Ehsan backed this out because it caused a Win7 reftest orange :-/
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 17

8 years ago
FWIW, I saw the orange on both debug and opt builds.
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.
No longer blocks: 614720
You need to log in before you can comment on or make changes to this bug.