1366402 - Use AutoMemMap in the component loader

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement)

Description

[Kris Maglione [:kmag]]

The plain file script loading code in mozJSComponentLoader duplicates a lot of the same file opening/mapping/cleanup logic that's implemented in the auto AutoMemMap RAII class used by other parts of the loader. Reusing it there would allow us to delete a lot of redundant code.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1366402: Use AutoMemMap helper in mozJSComponentLoader.

This helper is already being used in the script preloader, and encapsulates
all of the memory mapping and RAII logic used to do the same in the component
loader. Reusing it there allows us to remove a lot of redundant code.

This is applied on top of the patches for bug 989373 in order to avoid
conflicts.

Review commit: https://reviewboard.mozilla.org/r/141320/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/141320/

Comment 2

[Andrew McCreight [:mccr8]]

Comment on attachment 8869759 [details]
Bug 1366402: Use AutoMemMap helper in mozJSComponentLoader.

https://reviewboard.mozilla.org/r/141320/#review145618

Nice. I always just ignored that big chunk of code.

::: js/xpconnect/loader/ScriptPreloader-inl.h:24
(Diff revision 1)
>  
>  #include <prio.h>
>  
>  namespace mozilla {
> +
> +// A specialization of GenericErrorResult which auto-converts to a nsresult.

Maybe start the comment with XXX to indicate an issue that should be eventually fixed?

::: js/xpconnect/loader/mozJSComponentLoader.cpp:42
(Diff revision 1)
>  #include "xpcpublic.h"
>  #include "nsContentUtils.h"
>  #include "nsXULAppAPI.h"
>  #include "WrapperFactory.h"
>  
> +#include "AutoMemMap.h"

I think this should be mozilla/devtools/AutoMemMap.h based on the EXPORTS in moz.build.

Comment 3

[Kris Maglione [:kmag]]

Reviewboard isn't showing this review, so replying in Bugzilla.

(In reply to Andrew McCreight [:mccr8] from comment #2)
> ::: js/xpconnect/loader/ScriptPreloader-inl.h:24
> (Diff revision 1)
> >  
> >  #include <prio.h>
> >  
> >  namespace mozilla {
> > +
> > +// A specialization of GenericErrorResult which auto-converts to a nsresult.
> 
> Maybe start the comment with XXX to indicate an issue that should be
> eventually fixed?

I should hopefully be able to get bug 1366511 landed in the next week or two,
so was planning to remove it in whichever patch landed second. I'll update the
comment with an XXX and bug number.

> ::: js/xpconnect/loader/mozJSComponentLoader.cpp:42
> (Diff revision 1)
> >  #include "xpcpublic.h"
> >  #include "nsContentUtils.h"
> >  #include "nsXULAppAPI.h"
> >  #include "WrapperFactory.h"
> >  
> > +#include "AutoMemMap.h"
> 
> I think this should be mozilla/devtools/AutoMemMap.h based on the EXPORTS in
> moz.build.

There's a separate AutoMemMap.h in the loader directory, based on but slightly
different from the devtools version, that I added in bug 1359653 (since I didn't
feel comfortable using devtools helpers in XPConnect code).

I'm planning to export this header and update the devtools code to use the
XPConnect version sometime soon.

Comment 4

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d78fccf03b43ff103be9339a705402b7da66602
Bug 1366402: Use AutoMemMap helper in mozJSComponentLoader. r=mccr8

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4d78fccf03b4