Closed Bug 707090 Opened 13 years ago Closed 13 years ago

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

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla11
Tracking Flags:
Tracking Status
firefox11 - fixed

People

(Reporter: philikon, Assigned: fabrice)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

patch
(deleted), patch
mfinkle
: review+
Details | Diff | Splinter Review
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()!
In general, could we add some aborts to prevent any new main thread I/O?
How would you distinguish the new main thread i/o from the existing main thread i/o?
I don't know :) That is why I asked if we could add something for the new stuff. Maybe static analysis could help here?
(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.
(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...
This was added by bug 697383. Can we either back that out or fix it before we release this API?
status-firefox11: --- → affected
tracking-firefox11: --- → ?
Attached patch patch (obsolete) (deleted) — — Splinter Review
This patch adds a _loadJSONAsync() method that is used both in init() and in _readManifest()
Assignee: nobody → fabrice
Attachment #578656 - Flags: review?(philipp)
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?
Attachment #578656 - Flags: review?(philipp) → feedback-
Attached patch patch (deleted) — — Splinter Review
Here's a fully async version, with callbacks everywhere.
Attachment #578656 - Attachment is obsolete: true
Attachment #578931 - Flags: review?(philipp)
Attachment #578931 - Flags: review?(philipp) → review?(mark.finkle)
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.
Attachment #578931 - Flags: review?(mark.finkle) → review+
Mark, the getManifestFor() is a helper for chrome consumers of the API (currently it's used to implement launch() on desktop)
https://hg.mozilla.org/mozilla-central/rev/d524a35f7c7c
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: