707090 - DOMApplicationRegistry (Webapps.jsm) must not do any mainthread I/O

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Philipp von Weitershausen [:philikon]]

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][[e-mail]]]

In general, could we add some aborts to prevent any new main thread I/O?

Comment 2

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

How would you distinguish the new main thread i/o from the existing main thread i/o?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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]

(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]

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é]

Attachment: patch

This patch adds a _loadJSONAsync() method that is used both in init() and in _readManifest()

Comment 8

[Philipp von Weitershausen [:philikon]]

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é]

Attachment: patch

Here's a fully async version, with callbacks everywhere.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

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é]

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é]

pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d524a35f7c7c

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d524a35f7c7c