Last Comment Bug 707090 - DOMApplicationRegistry (Webapps.jsm) must not do any mainthread I/O
: DOMApplicationRegistry (Webapps.jsm) must not do any mainthread I/O
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: [:fabrice] Fabrice Desré
:
Mentors:
Depends on:
Blocks: 697383
  Show dependency treegraph
 
Reported: 2011-12-02 00:31 PST by Philipp von Weitershausen [:philikon]
Modified: 2012-02-01 13:07 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
fixed


Attachments
patch (3.03 KB, patch)
2011-12-02 11:17 PST, [:fabrice] Fabrice Desré
philipp: feedback-
Details | Diff | Review
patch (5.46 KB, patch)
2011-12-04 13:13 PST, [:fabrice] Fabrice Desré
mark.finkle: review+
Details | Diff | Review

Description Philipp von Weitershausen [:philikon] 2011-12-02 00:31:12 PST
DOMApplicationRegistry._readManifest() does a blocking file read, as far as I can tell on the mainthread. Not good. I strongly recommend using NetUtil.asyncFetch(), which is already used in DOMApplicationRegistry.init()!
Comment 1 Olli Pettay [:smaug] 2011-12-02 08:22:32 PST
In general, could we add some aborts to prevent any new main thread I/O?
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-02 08:23:29 PST
How would you distinguish the new main thread i/o from the existing main thread i/o?
Comment 3 Olli Pettay [:smaug] 2011-12-02 08:26:12 PST
I don't know :) That is why I asked if we could add something for the new stuff. Maybe static
analysis could help here?
Comment 4 Philipp von Weitershausen [:philikon] 2011-12-02 09:03:53 PST
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2)
> How would you distinguish the new main thread i/o from the existing main
> thread i/o?

I like the idea, but it's off-topic for this bug. I filed bug 707196 to get this discussion going.
Comment 5 Shawn Wilsher :sdwilsh 2011-12-02 10:55:34 PST
(In reply to Olli Pettay [:smaug] from comment #1)
> In general, could we add some aborts to prevent any new main thread I/O?
We have too much main thread I/O still, sadly.  It'd help if reviewers were a bit more strict about this...
Comment 6 Shawn Wilsher :sdwilsh 2011-12-02 10:57:50 PST
This was added by bug 697383.  Can we either back that out or fix it before we release this API?
Comment 7 [:fabrice] Fabrice Desré 2011-12-02 11:17:20 PST
Created attachment 578656 [details] [diff] [review]
patch

This patch adds a _loadJSONAsync() method that is used both in init() and in _readManifest()
Comment 8 Philipp von Weitershausen [:philikon] 2011-12-02 11:32:29 PST
Comment on attachment 578656 [details] [diff] [review]
patch

>+    while (!loaded)
>+      Services.tm.currentThread.processNextEvent(true);

Please no event loop spinning either. It's being done in Sync and we've spent the last 8 months trying to get rid of it...

Can't you just make the manifest loading operation take a callback?
Comment 9 [:fabrice] Fabrice Desré 2011-12-04 13:13:46 PST
Created attachment 578931 [details] [diff] [review]
patch

Here's a fully async version, with callbacks everywhere.
Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-05 12:36:14 PST
Comment on attachment 578931 [details] [diff] [review]
patch

Boy, the enumerateX APIs complicate this. Also, is getManifestFor part of the API or a helper? I don't see it used in the code.
Comment 11 [:fabrice] Fabrice Desré 2011-12-05 13:26:19 PST
Mark, the getManifestFor() is a helper for chrome consumers of the API (currently it's used to implement launch() on desktop)
Comment 12 [:fabrice] Fabrice Desré 2011-12-05 18:29:51 PST
pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d524a35f7c7c
Comment 13 Ed Morley [:emorley] 2011-12-06 10:06:08 PST
https://hg.mozilla.org/mozilla-central/rev/d524a35f7c7c

Note You need to log in before you can comment on or make changes to this bug.