Closed Bug 1497707 Opened Last year Closed 11 months ago

Various module loader cleanups

Categories

(Core :: XPCOM, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(5 files)

The bulk of changes I have in the patches for bug 1492584 are code cleanups, so I should work to get those landed while I figure out what the issue is with the final part. See bug 1492584 comment 1 for what these cleanups are. The major win here is to get rid of the abstraction that the module loader uses where it semi-supports loading from a variety of different extensions, while in practice on JS is actually implemented.
Priority: -- → P3
This interface is only used for a few testing functions. Just move
them to Cu.
This allows some code to be deleted, including a KnownModule ctor.

Depends on D8168
JS is the only file extension actually supported, and there are a few
layers of cruft that can be eliminated if we specialize it.

This eliminates one XPCOM registration of the JS component loader.

Depends on D8170
Now that the XPCOM component loader infrastructure has stopped
pretending to support other file extensions, this intermediate
interface is no longer needed.

Depends on D8171
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/80bf9ddf5bed
part 1 - Get rid of xpcIJSModuleLoader r=kmag
https://hg.mozilla.org/integration/autoland/rev/f37f2d39ec9c
part 2 - The second argument to nsComponentManagerImpl::RegisterModule is always null r=froydnj
https://hg.mozilla.org/integration/autoland/rev/32595f9e73d3
part 3 - Remove various unused things from nsComponentManager r=froydnj
https://hg.mozilla.org/integration/autoland/rev/11c813f192e2
part 4 - Only support loading JS files in the component manager r=froydnj
https://hg.mozilla.org/integration/autoland/rev/bb1b80139e37
part 5 - Inline mozilla::ModuleLoader into mozJSComponentLoader r=froydnj
Backout by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c73a5da46b9f
Backed out 5 changesets for android mass failures. CLOSED TREE
Hmm I did run this through try for Android XPCShell tests, but not other tests. Looks like XPCShell tests pass while the other ones fail.

It looks like this is hitting the same problems I was seeing with bug 1492584 where the JS engine isn't being start soon enough. I can only think this must be a result of part 4, as the other 3 seem completely benign.

https://treeherder.mozilla.org/logviewer.html#?job_id=206222823&repo=autoland&lineNumber=1351
I think I figured out why this crashes, and only on Android. On Android, nsIDirectoryServiceProvider is implemented in JS, which gets loaded a few lines before we initialize the JS loader, in nsDirectoryService::RegisterCategoryProviders. Prior to my patch, when it loaded up a component implemented in JS, it would go through nsComponentManagerImpl::LoaderForExtension(), which would call do_GetServiceFromCategory(), which would load the layout module, which loads XPConnect. This may have worked before I fixed up various manifest errors, because we'd instead hit the manifest errors.

Anyways, the current set up with part 4 sounds too fragile. Before, if you were using a JS component, you'd always start up XPConnect first. Now, it has to be poked some other way first, or you crash.
Attachment #9015734 - Attachment description: Bug 1497707, part 1 - Get rid of xpcIJSModuleLoader → Bug 1497707, part 1 - Get rid of xpcIJSModuleLoader.
Attachment #9015735 - Attachment description: Bug 1497707, part 2 - The second argument to nsComponentManagerImpl::RegisterModule is always null → Bug 1497707, part 2 - The second argument to nsComponentManagerImpl::RegisterModule is always null.
Attachment #9015736 - Attachment description: Bug 1497707, part 3 - Remove various unused things from nsComponentManager → Bug 1497707, part 3 - Remove various unused things from nsComponentManager.
Attachment #9015737 - Attachment description: Bug 1497707, part 4 - Only support loading JS files in the component manager → Bug 1497707, part 4 - Only support loading JS files in the component manager.
Attachment #9015738 - Attachment description: Bug 1497707, part 5 - Inline mozilla::ModuleLoader into mozJSComponentLoader → Bug 1497707, part 5 - Inline mozilla::ModuleLoader into mozJSComponentLoader.
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/26d5c8531a08
part 1 - Get rid of xpcIJSModuleLoader. r=kmag
https://hg.mozilla.org/integration/autoland/rev/bd13c578aceb
part 2 - The second argument to nsComponentManagerImpl::RegisterModule is always null. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/3db6926e3e44
part 3 - Remove various unused things from nsComponentManager. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/e44f3f8cde9c
part 4 - Only support loading JS files in the component manager. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/ded047a09c69
part 5 - Inline mozilla::ModuleLoader into mozJSComponentLoader. r=froydnj
I ended up working around the Android crash by moving up the jsloader load a couple of lines, to right after we initialize the component loader. My patches in bug 1492584 move it inside component loader initialization, so that seemed reasonable to do.
You need to log in before you can comment on or make changes to this bug.