961777 - Firefox doesn't work with jsloader.reuseGlobal=true

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Alexandre Poirot [:ochameau]]

Firefox breaks in mutliple ways when we enable this pref.
I don't think it is really important to enable it for Firefox itself,
but for the mulet (a merge of b2g desktop and firefox),
it becomes important to keep this pref on in order to behave like b2g on device.

We should find a way to make this mode work, at least with the b2g content we are loading into firefox.

Here is some options on top of my mind:
 - limit the scope of this pref,
 - tune its behavior to prevent breaking so many jsm,
 - fix all jss
 - ?

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Disable reuseGlobal

Patch for local branch, just to make it work when building such "mulet".

Comment 2

[Julien Wajsberg [:julienw]]

This bug could also be called "why do I need to bind my exported symbols to `this`?".

Comment 3

[Alexandre Poirot [:ochameau]]

So the issue is that most firefox jsms are written as regular jsm:

  var EXPORTED_SYMBOLS=["foo"];
  var foo={};

But with reuseGlobal, on b2g, you have to do:

  this.EXPORTED_SYMBOLS=["foo"];
  this.foo={};

I wish we could keep reuseGlobal turned on in the mulet to better match what is being done on device. So ideally we would get rid of this difference.
I started asking around to gabor who asked to jorendorff and it looks like there is some ways to automagically pull symbols from the scope of the anonymous function we are using for the reuseGlobal thing.

gabor: we load then compile it to a function or to a script here: http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/mozJSComponentLoader.cpp#952
gabor: and then cache it a few lines later
gabor: execute the script / call the function with the fake global
gabor: and then try to find the exported symbols in it
gabor: so the idea is that load this magic module once, and then call the function that is compiled from its script after this one: http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/moz
JSComponentLoader.cpp#1017
gabor: if the Parser API is so slow that it causes a problem here, that should be addressed, but I would doubt

I raised some concerns to gabor as it sounds (to me) like an costly operation, but as I don't know much about xpconnect and js internals... I would prefer experts to debate and hopefully address this issue!

And btw, if you don't know about the mulet, you may look at bug 943878. Jonas is about to send a mail to officialize this project.

Comment 4

[Gabor Krizsanits (INACTIVE)]

So with the current set up I think there are two ways to go. I'm not a JSAPI guru, so which one is the better approach is beyond me to decide. One is to somehow reference the closure in which the JS function that is created for the jsm was executed, and somehow lookup symbols in that closure (all this in C++). I'm not sure the current JS API allows such thing, according to Jorendorff the Debugger does something similar, so it's not completely impossible, but he was against this approach.

Jasons idea was to simply add some additional script to the end of each jsm in case of reuseGlobal, and then do the lookup with the help of the Parser API. I don't know much about the Parser API, and its performance at all. I'm less pessimistic about it than Alex, but I can be totally wrong.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

FWIW, I think we should look into this, but I don't think we should let it block bug 943878. I *think* we can live with reusing globals on-device but not on-desktop for a bit. Though it's definitely scary.

Comment 6

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

I'm not sure what the question you want me to answer is.  We do what we do today because there is no other API to get the relevant symbols out of JS.

Comment 7

[Jason Orendorff [:jorendorff]]

I admit I might not understand what's going on here exactly. But here's the kind of code I had in mind.


// Return an array of all the names declared in the given script `code`.
function getScriptDeclaredGlobals(code) {
    var names = [];

    function noteDeclaredNames(id) {
        if (id.type === "Identifier") {
            names.push(id.name);
        } else if (id.type === "ArrayPattern") {
            for (var e of id.elements)
                noteDeclaredNames(e);
        } else if (id.type === "ObjectPattern") {
            for (var prop of id.properties)
                noteDeclaredNames(prop.value);
        }
    }

    for (var stmt of Reflect.parse(code).body) {
        if (stmt.type === "VariableDeclaration") {
            for (var decl of stmt.declarations)
                noteDeclaredNames(decl.id);
        } else if (stmt.type === "FunctionDeclaration") {
            names.push(stmt.id.name);
        }
    }
    return names;
}

// Append to `code` some additional code to add global bindings for all
// functions, variables, and constants declared in it.
function modifyJSMForReuseGlobal(code) {
    for (var name of getScriptDeclaredGlobals(code))
        code += "this." + name + " = " + name + ";\n";
    return code;
}

Comment 8

[Jason Orendorff [:jorendorff]]

Of course the exact code you want to append there depends on the exact behavior you're looking for. Perhaps it should be more like

    code += "if (EXPORTED_SYMBOLS.indexOf('" + name + "') !== -1) this." + name + " = " + name + ";\n";

or maybe you want to use Object.defineProperty to define a getter. But whatever, that's easy to tweak.

It's hard to do the same stuff from C++. So the full solution would be something like this. Use JS_EvaluateScript to install the above code and this into your reusable global:

    function installJSM(code) {
        Function(modifyJSMForReuseGlobal(code))();
    }

and then to install a module, use JS_CallMethod to call "installJSM" (instead of JS_EvaluateScript or whatever you're using now).

If I am completely confused about what you're trying to do, track me down on IRC...

Comment 9

[Jason Orendorff [:jorendorff]]

If it's too slow, it could be done only for third-party code and all our in-tree code could just be fixed.

Comment 10

[Alexandre Poirot [:ochameau]]

I talked to various people and it looks like, it isn't 100% clear why we can or can't fetch exported symbols. But mozSubScriptLoader.loadSubScript does fetch symbols of the evaluated script, even when passing a custom global/target object. We may just tweak Cu.import to have same behavior of loadSubScript...

Or may be we can just get rid of this!

Bobby, in bug 798491 you mentioned getting rid of it.
Do you think we can get rid of it while still addressing CCW overhead?

Comment 11

[Bobby Holley (:bholley)]

(In reply to Alexandre Poirot (:ochameau) from comment #10)
> Bobby, in bug 798491 you mentioned getting rid of it.
> Do you think we can get rid of it while still addressing CCW overhead?

I'm somewhat optimistic - somebody just needs to measure it. What kind of memory regression do we get if we flip off reuseGlobal in b2g?

Comment 12

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #11)
> (In reply to Alexandre Poirot (:ochameau) from comment #10)
> > Bobby, in bug 798491 you mentioned getting rid of it.
> > Do you think we can get rid of it while still addressing CCW overhead?
> 
> I'm somewhat optimistic - somebody just needs to measure it. What kind of
> memory regression do we get if we flip off reuseGlobal in b2g?

We could just put everything in the same zone instead of sharing global, but there is another cost of this change. That would introduce a bunch of (transparent) cross compartment wrappers. I'm not too worried about the memory footprint of those wrappers, but call paths can get a lot more expensive in some cases.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #12)
> We could just put everything in the same zone instead of sharing global, but
> there is another cost of this change. That would introduce a bunch of
> (transparent) cross compartment wrappers. I'm not too worried about the
> memory footprint of those wrappers, but call paths can get a lot more
> expensive in some cases.

That's true. But I think the primary motivation for this stuff was memory, which is why we turned it on for b2g and not elsewhere.

In general, we should probably just try to avoid code patterns that do hot stuff cross-global, because those are going to suck if they ever end up running in a DOM window.

Comment 14

[Gregor Wagner [:gwagner]]

(In reply to Bobby Holley (:bholley) from comment #11)
> (In reply to Alexandre Poirot (:ochameau) from comment #10)
> > Bobby, in bug 798491 you mentioned getting rid of it.
> > Do you think we can get rid of it while still addressing CCW overhead?
> 
> I'm somewhat optimistic - somebody just needs to measure it. What kind of
> memory regression do we get if we flip off reuseGlobal in b2g?

Kyle has all the numbers but I remember it being a huge win. With our 128MB device we are down to optimizing KBs now instead of MBs so I don't think we can disable it again.

Comment 15

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

There are numbers in bug 989373.

Comment 16

[Bobby Holley (:bholley)]

(In reply to Gregor Wagner [:gwagner] from comment #14)
> Kyle has all the numbers but I remember it being a huge win. With our 128MB
> device we are down to optimizing KBs now instead of MBs so I don't think we
> can disable it again.

The huge win you remember was pre-zones (which let many of the relevant data structures be shared between compartments). So whatever the numbers are now, the ones you remember aren't really relevant anymore.

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: Disable reuseGlobal on Mulet

Here is a more sane patch.
Waiting for build system patch to land before asking review
as it may change...

Comment 18

[Alexandre Poirot [:ochameau]]

Comment on attachment 8404703 [details] [diff] [review]
Disable reuseGlobal on Mulet

That makes me sad to introduce this difference between device and mulet,
but I don't know how to fix that issue otherwise.

I tried to talk with jimb and khuey about why reuseGlobal can't fetch symbols of the JSM, without much results. It was beyond my knowledges to understand why loadSubScript do similar things than JSM with reuseGlobal mode *but* is able to fetch the inner symbols.

# myfile.js:
var foo = {};

var g = {};
Cu.loadSubScript("myfile.js", g);

g.foo is visible, where myfile.js is evaluated againt g as "kind of global".
It really looks like what Cu.import does with reuseGlobal,
but there is no issue in pulling myfile.js symbols...

Hopefully, we can soon get rid of reuseGlobal. Otherwise that would be great to address the symbol issue in the meantime.

Comment 19

[Bobby Holley (:bholley)]

Comment on attachment 8404703 [details] [diff] [review]
Disable reuseGlobal on Mulet

Review of attachment 8404703 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +352,5 @@
>      // XXXkhuey B2G child processes have some sort of preferences race that
>      // results in getting the wrong value.
>  #ifdef MOZ_B2G
> +    // But we don't want that on Firefox Mulet as it break most Firefox JSMs...
> +#ifndef MOZ_MULET

Can we make this:

#if defined(MOZ_B2G) && !defined(MOZ_MULET)

?

Comment 20

[Alexandre Poirot [:ochameau]]

Attachment: Disable reuseGlobal on Mulet

(In reply to Bobby Holley (:bholley) from comment #19)
> Can we make this:
> 
> #if defined(MOZ_B2G) && !defined(MOZ_MULET)
> 
> ?

Sure!

https://tbpl.mozilla.org/?tree=Try&rev=31fac2cf0574

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78bb49b681db

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/78bb49b681db