980752 - addon-sdk/source/lib/toolkit/loader.js calls Object.freeze(Object.prototype) which can break other JS modules when jsloader.reuseGlobal is true (always true in b2g), matters because Marionette triggers the loader

Status: RESOLVED INCOMPLETE

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Andrew Sutherland [:asuth] (he/him)]

Bug 798491 introduced a mechanism to reduce B2G memory by loading all JS modules in the same compartment.  (Discussion context: https://groups.google.com/forum/#!msg/mozilla.dev.platform/Ic2YmqbRyIE/ApBWouLoMQkJ)  An inherent side effect of this is that all JS modules share the same global.  The mechanism is theoretically controlled by the preference "jsloader.reuseGlobal" but is currently hard-coded to true for B2G because of a multi-process issue:
dxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/mozJSComponentLoader.cpp#352

The freezing logic is here and tries to freeze pretty much everything it can:
http://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/toolkit/loader.js#94

The rationale seems to be that the loader can be used in multiple situations and one of those is a case where hostile JS code could be loaded into the same context.  Arguably the loader should not be loading hostile code when it is running as a Chrome privileged module, and so should skip all the freezing in that case.


This can be a problem for idiomatic code that does things like:
Foo.prototype.toString = function() {...};

The descriptor for toString ends up as {"configurable":false,"enumerable":false,"writable":false} and this makes attempts like that sad, apparently.


This can affect B2G with marionette enabled/active.  I instrumented the loader with a 'debugger' statement, broke on that, and called DumpJSStack() and we see:

0 Loader(require = [function], exports = [object Object], module = [object Object]) ["resource://gre/modules/commonjs/toolkit/loader.js":97]
1 anonymous(id = "loader", factory = [function]) ["resource://gre/modules/commonjs/toolkit/loader.js":16]
    this = [object FakeBackstagePass]
2 anonymous() ["resource://gre/modules/commonjs/toolkit/loader.js":26]
    this = [object FakeBackstagePass]
3 anonymous() ["resource://gre/modules/devtools/Loader.jsm":24]
    this = [object FakeBackstagePass]
4 anonymous() ["chrome://marionette/content/marionette-server.js":42]
    this = [object FakeBackstagePass]
5 mc_init() ["file:///.../obj-b2g/dist/bin/components/marionettecomponent.js":162]
    this = [object Object]
6 mc_observe(aSubject = null, aTopic = "final-ui-startup", aData = null) ["file:///.../obj-b2g/dist/bin/components/marionettecomponent.js":106]


This was detected and initially investigated by :jrburke when he noticed a Gaia e-mail app test failure that started happening very recently.  Maybe bug 972925 or follow-ons?

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Andrew Sutherland (:asuth) from comment #0)
> Bug 798491 introduced a mechanism to reduce B2G memory by loading all JS
> modules in the same compartment.  (Discussion context:
> https://groups.google.com/forum/#!msg/mozilla.dev.platform/Ic2YmqbRyIE/
> ApBWouLoMQkJ)  An inherent side effect of this is that all JS modules share
> the same global.  The mechanism is theoretically controlled by the
> preference "jsloader.reuseGlobal" but is currently hard-coded to true for
> B2G because of a multi-process issue:
> dxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/
> mozJSComponentLoader.cpp#352
> 
> The freezing logic is here and tries to freeze pretty much everything it can:
> http://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/toolkit/
> loader.js#94
> 
> The rationale seems to be that the loader can be used in multiple situations
> and one of those is a case where hostile JS code could be loaded into the
> same context.  Arguably the loader should not be loading hostile code when
> it is running as a Chrome privileged module, and so should skip all the
> freezing in that case.

Well reason is to not allow any module loaded by an add-on (which maybe authored by
third praty) to affect a behavior of the loader or add-on. Not that most add-on modules
are loaded as chrome privileged. 

> 
> 
> This can be a problem for idiomatic code that does things like:
> Foo.prototype.toString = function() {...};
> 
> The descriptor for toString ends up as
> {"configurable":false,"enumerable":false,"writable":false} and this makes
> attempts like that sad, apparently.
> 

It's very much possible to define Foo.prototype.toString though like:

Object.defineProperty(Foo.prototype, "toString", {
  value: function() { ... }  
})

> 
> This can affect B2G with marionette enabled/active.  I instrumented the
> loader with a 'debugger' statement, broke on that, and called DumpJSStack()
> and we see:
> 
> 0 Loader(require = [function], exports = [object Object], module = [object
> Object]) ["resource://gre/modules/commonjs/toolkit/loader.js":97]
> 1 anonymous(id = "loader", factory = [function])
> ["resource://gre/modules/commonjs/toolkit/loader.js":16]
>     this = [object FakeBackstagePass]
> 2 anonymous() ["resource://gre/modules/commonjs/toolkit/loader.js":26]
>     this = [object FakeBackstagePass]
> 3 anonymous() ["resource://gre/modules/devtools/Loader.jsm":24]
>     this = [object FakeBackstagePass]
> 4 anonymous() ["chrome://marionette/content/marionette-server.js":42]
>     this = [object FakeBackstagePass]
> 5 mc_init()
> ["file:///.../obj-b2g/dist/bin/components/marionettecomponent.js":162]
>     this = [object Object]
> 6 mc_observe(aSubject = null, aTopic = "final-ui-startup", aData = null)
> ["file:///.../obj-b2g/dist/bin/components/marionettecomponent.js":106]
> 
> 
> This was detected and initially investigated by :jrburke when he noticed a
> Gaia e-mail app test failure that started happening very recently.  Maybe
> bug 972925 or follow-ons?

BTW if module trying to set non configurable property was running under "use strict"
mode exception would have being thrown rather then having silent error to define
property.


I'm not sure I have any good solutions in mind. I thin it would be most reasonable to not make assumptions weather toString is configurable or not and just use `Object.defineProperty`. If this is unreasonable option we could probably modify loader code to unfreeze Object.prototype and just not rely on any of it methods.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #1)
> > This can be a problem for idiomatic code that does things like:
> > Foo.prototype.toString = function() {...};
> > 
> > The descriptor for toString ends up as
> > {"configurable":false,"enumerable":false,"writable":false} and this makes
> > attempts like that sad, apparently.
> > 
> 
> It's very much possible to define Foo.prototype.toString though like:
> 
> Object.defineProperty(Foo.prototype, "toString", {
>   value: function() { ... }  
> })

Yes, there are idioms to work-around it once you've identified the breakage.  But as you note, if code isn't using "use strict" then the problem becomes a silent failure which can be even harder to diagnose.
 
> I'm not sure I have any good solutions in mind. I thin it would be most
> reasonable to not make assumptions weather toString is configurable or not
> and just use `Object.defineProperty`. If this is unreasonable option we
> could probably modify loader code to unfreeze Object.prototype and just not
> rely on any of it methods.

The simplest/hackiest solution is to just check the pref "jsloader.reuseGlobal" and not mess with the globals if true.  (Noting that the preference does not actually do anything right now because the underlying code making the decision is hobbled by the preferences not being loaded when it first runs and so the value ends up being hardcoded.)  The scenario the loader is trying to protect against does not exist in B2G and likely never will.  In all other gecko scenarios, the loader's habit of freezing things when in its own compartment is harmless.

Comment 3

[Jason Orendorff [:jorendorff]]

(In reply to Andrew Sutherland (:asuth) from comment #0)
> This can be a problem for idiomatic code that does things like:
> Foo.prototype.toString = function() {...};

I think this is a JS engine bug. Investigating.

Comment 4

[Jason Orendorff [:jorendorff]]

My mistake. ES5 and ES6 both say this should throw in strict mode (which is what we do). Bug in Chrome that it doesn't do the same.

Comment 5

[Jason Orendorff [:jorendorff]]

Proposed changing the JS standard to accommodate this:
  http://esdiscuss.org/topic/set-and-inherited-readonly-data-properties

Don't get your hopes up.

Comment 6

[Jason Orendorff [:jorendorff]]

Yeah, that didn't go anywhere. Oh well.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Jason Orendorff [:jorendorff] from comment #6)
> Yeah, that didn't go anywhere. Oh well.

Thanks for trying!

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Andrew Sutherland (:asuth) from comment #2)
> (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment
> #1)
> > > This can be a problem for idiomatic code that does things like:
> > > Foo.prototype.toString = function() {...};
> > > 
> > > The descriptor for toString ends up as
> > > {"configurable":false,"enumerable":false,"writable":false} and this makes
> > > attempts like that sad, apparently.
> > > 
> > 
> > It's very much possible to define Foo.prototype.toString though like:
> > 
> > Object.defineProperty(Foo.prototype, "toString", {
> >   value: function() { ... }  
> > })
> 
> Yes, there are idioms to work-around it once you've identified the breakage.
> But as you note, if code isn't using "use strict" then the problem becomes a
> silent failure which can be even harder to diagnose.
>  
> > I'm not sure I have any good solutions in mind. I thin it would be most
> > reasonable to not make assumptions weather toString is configurable or not
> > and just use `Object.defineProperty`. If this is unreasonable option we
> > could probably modify loader code to unfreeze Object.prototype and just not
> > rely on any of it methods.
> 
> The simplest/hackiest solution is to just check the pref
> "jsloader.reuseGlobal" and not mess with the globals if true.  (Noting that
> the preference does not actually do anything right now because the
> underlying code making the decision is hobbled by the preferences not being
> loaded when it first runs and so the value ends up being hardcoded.)  The
> scenario the loader is trying to protect against does not exist in B2G and
> likely never will.  In all other gecko scenarios, the loader's habit of
> freezing things when in its own compartment is harmless.

Andrew if you wanna submit a pull request that adds an option to a loader to prevent freezing globals ?
I think that is reasonable option.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #8)
> Andrew if you wanna submit a pull request that adds an option to a loader to
> prevent freezing globals ?
> I think that is reasonable option.

Sure.  Can you clarify on what you mean by "an option"?  Were you agreeing with one of these proposals, or did you want something else like requiring the code causing the loader to run to pass a flag / set some other configuration indicator?
- check jsloader.reuseGlobal pref
- check the principal it's operating with

Also, when you say pull request, do you mean an actual pull request like the add-on SDK used to use when it was tree-external, or just a patch in bugzilla?

Comment 10

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Andrew Sutherland (:asuth) from comment #9)
> (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment
> #8)
> > Andrew if you wanna submit a pull request that adds an option to a loader to
> > prevent freezing globals ?
> > I think that is reasonable option.
> 
> Sure.  Can you clarify on what you mean by "an option"?  Were you agreeing
> with one of these proposals, or did you want something else like requiring
> the code causing the loader to run to pass a flag / set some other
> configuration indicator?
> - check jsloader.reuseGlobal pref
> - check the principal it's operating with

What I meant is an option to a Loader constructor:
https://github.com/Gozala/addon-sdk/blob/master/lib/toolkit/loader.js#L396

Maybe something like `Loader({ dontFreeze: true, .... })`

> 
> Also, when you say pull request, do you mean an actual pull request like the
> add-on SDK used to use when it was tree-external, or just a patch in
> bugzilla?

We still use github as our canonical repository. But if you prefer patch that's fine too.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Incidentally, I appear to be hitting this trying to run gaia-ui-tests locally to deal with bug 1033098.

Comment 12

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562