1159422 - Disable 'javascript.options.discardSystemSource' by default

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect, P1)

Description

[Doug Sherk (:drs) (inactive)]

Enabling 'javascript.options.discardSystemSource' is breaking some privileged/certified Spark apps. Since our hardware going forward will have more memory, this optimization isn't necessary anymore.

Comment 1

[Doug Sherk (:drs) (inactive)]

Attachment: Disable 'javascript.options.discardSystemSource' by default.

Comment 2

[Jet Villegas (inactive)]

This was an effective optimization for low-mem devices, but also helps other devices. Devs hacking on privileged apps know how to flip a pref, so why not keep the memory win?

Comment 3

[Jet Villegas (inactive)]

See also: bug 1134933

Comment 4

[Doug Sherk (:drs) (inactive)]

(In reply to Jet Villegas (:jet) from comment #2)
> This was an effective optimization for low-mem devices, but also helps other
> devices. Devs hacking on privileged apps know how to flip a pref, so why not
> keep the memory win?

The apps that I'm referring to will be shipping as part of the next version of Firefox OS, called Spark, so they need to work out of the box. These apps pull in many libraries, compared to the existing stock Gaia apps. These libraries depend on functionality that is broken by this pref being enabled.

Here are some of the apps, for reference:
https://github.com/fxos/sharing
https://github.com/fxos/customizer
https://github.com/fxos/directory

I'd be fine with fixing this with bug 1134933 as well. But since there's no patch or work on that bug, it makes more sense to me to land this for now, and then re-enable 'javascript.options.discardSystemSource' when bug 1134933 lands.

Comment 5

[[:fabrice] Fabrice Desré]

I'd like to understand what in these apps needs eval() or toSource() first.

Comment 6

[Justin D'Arcangelo [:justindarc]]

(In reply to Fabrice Desré [:fabrice] from comment #5)
> I'd like to understand what in these apps needs eval() or toSource() first.

At the very least, we use it in Customizer to lazy load the bulk of the dependencies after the gesture to open the Customizer has been detected. Right now, this is done by injecting something like:

```
function lazyLoadDeps(){/*
 ...
*/}
```

...where we grab the source inside `lazyLoadDeps()` and `eval()` it.

However, I also think I remember RequireJS and/or Alameda needing `.toSource()`.

Comment 7

[James Burke [:jrburke]]

The RequireJS/alameda loaders only need Function.toString if the dependencies are not traced out in a build and placed as a dependency array. A brief test of the directory app it looks like the source translation done at build time does write out the dependencies in an array so should be OK with discardSystemSource set to true.

If using r.js optimizer, this can be ensured when doing a build with the normalizeDirDefines option:
https://github.com/mozilla-b2g/gaia/blob/master/apps/email/build/email.build.js#L16

It does mean that a build is required in order for this to work with discardSystemSource set to true, which to me takes away one of the benefits of web development, not needing a build for dev/debug cycles.

So in general I prefer if discardSystemSource was not set to true, but can understand if the memory gains require it. For the specific dev/debug cycles, it may be enough to ask the developer to flip that switch. It is an unusual thing to need to do though, so it normally trips up developers using a module system at first.

Comment 8

[Doug Sherk (:drs) (inactive)]

(In reply to James Burke [:jrburke] from comment #7)

James, I generally agree with you that we should just disable this pref, but we should also know why we need it disabled so that these apps can be uploaded to Marketplace and be run on devices that have this pref disabled.

I'm getting this error on startup with 'javascript.options.discardSystemSource' enabled:
W/Hackerplace( 3145): [JavaScript Error: "Error: Load failed: gaia-component: app://77267e5f-371e-8440-bcf5-9e44dc627c39/components/gaia-component/gaia-component.js" {file: "app://77267e5f-371e-8440-bcf5-9e44dc627c39/js/initapp.js" line: 893}]
W/Hackerplace( 3145): [JavaScript Error: "Error: Load failed: components/gaia-header/dist/gaia-header: app://77267e5f-371e-8440-bcf5-9e44dc627c39/components/gaia-header/dist/gaia-header.js" {file: "app://77267e5f-371e-8440-bcf5-9e44dc627c39/js/initapp.js" line: 893}]

Here's that block of code:
https://pastebin.mozilla.org/8831686

It looks like alameda is failing to load gaia-component, and this might be the root of the problem. Could you take a look and see if you can isolate this any further?

Comment 9

[James Burke [:jrburke]]

Typically if toString source is missing, the error will be something like "module 'something' not loaded", where this error seems like the fetch/load for the resource failed.

What app was being used and steps did you do to get to this error, so I can try to reproduce?

Here is what I tried:

git clone --recursive [e-mail]:fxos/directory.git
cd directory
bower install
npm install
gulp

I then used a desktop nightly firefox, set javascript.options.discardSystemSource to true and loaded http://localhost:8000/, but I do not see that error.

Comment 10

[Doug Sherk (:drs) (inactive)]

(In reply to James Burke [:jrburke] from comment #9)

Yeah, I used the Hackerplace ("directory") app, but I installed it on an actual device using WebIDE, from the ./directory/dist/app folder. You should also run |npm install| before |bower install|. I otherwise used the exact same process as you. This repros on Flame.

Comment 11

[James Burke [:jrburke]]

I am having trouble reproducing, any other clues on repo branch to use (I was using directory master), or other actions besides just launching the app would be helpful.

I did these steps fresh, removed the clone I did before:

git clone --recursive [e-mail]:fxos/directory.git
cd directory
npm install
bower install
gulp

Used a Firefox Nightly desktop build that is up to date to launch WebIDE, selected "Open Packaged App" item to then select the directory/dist/app directory, and pushed that to a flame device, then launched the app. No errors in logcat or WebIDE and the app seemed fine.

I used a very fresh flash (done right before this test) of Flame-KK, master, engineering, full flash from pvt builds.

Comment 12

[Doug Sherk (:drs) (inactive)]

You can run |make reset-gaia| from a gaia:master clone, which will set the pref to `true`. Building a profile with |DEVICE_DEBUG=1| will set the pref to `false`, which I believe eng builds do by default. That may explain why you can't repro the issue.

Comment 13

[James Burke [:jrburke]]

Attachment: app.zip for directory/dist/app

:drs - A make reset-gaia did not seem to surface the error. I also tried a "user" (instead of "eng") flash of latest master image on the device to see if that could do it, but did not.

I wonder if it one of the npm or bower dependencies had a slight update to address the problem, and by me fetching it fresh, got that update? Just trying to figure out the discrepancy. 

Or if there is a way I can confirm what b2g is really using for about:config values, I can check that too.

The attachment is the dist/app directory contents that I am installing on the phone. Perhaps you can try it to see if it fails for you. If not, then perhaps a sub-dependency is our disconnect.

This was from a directory app master branch, on commit 4223eec70f2ec128832343cb1d0e517575f624bd

Comment 14

[Doug Sherk (:drs) (inactive)]

Interesting! Something that landed on m-c, or one of our dependencies, must have fixed this in the last few days. Thanks for investigating, and sorry for the false alarm. This has been an issue for us for months now, and it didn't occur to me to check that it could still repro before filing this.

Comment 15

[daniel92frei]

Firefox OS is dead, so, would it make sense to enable javascript.options.discardSystemSource by default?

Comment 16

[[:fabrice] Fabrice Desré]

(In reply to daniel92frei from comment #15)
> Firefox OS is dead, so, would it make sense to enable
> javascript.options.discardSystemSource by default?

Firefox OS is not dead, and it probably doesn't make sense to *enable* discardSystemSource by default.

Comment 17

[daniel92frei]

Ok, sorry. Yes, Firefox OS will be on Panasonic TVs, it's on some Smartphones. It's not in fact 'dead' but i think this article has a point.
http://www.quirksmode.org/blog/archives/2015/12/firefox_os_is_d.html

With 'enable' i meant 'set to "true"'. Can you please explain why 'it probably doesn't make sense'?

Comment 18

[[:fabrice] Fabrice Desré]

(In reply to daniel92frei from comment #17)
> Ok, sorry. Yes, Firefox OS will be on Panasonic TVs, it's on some
> Smartphones. It's not in fact 'dead' but i think this article has a point.
> http://www.quirksmode.org/blog/archives/2015/12/firefox_os_is_d.html

Bugzilla is not the place to comment on that, but the author clearly doesn't understand what makes or break the success of a mobile OS (hint: some 3rd party apps have more leverage than the OS itself).

> With 'enable' i meant 'set to "true"'. Can you please explain why 'it
> probably doesn't make sense'?

This feature was introduced to reduce memory usage on devices with only 128MB of RAM. Since we don't focus on these anymore we should really not turn it on by default anywhere.