Closed Bug 1392602 Opened 7 years ago Closed 7 years ago

Copy SDK loader to /devtools and strip useless features

Categories

(DevTools :: General, enhancement, P1)

enhancement

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

(Whiteboard: [nosdk])

Attachments

(10 files, 1 obsolete file)

59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
DevTools still uses SDK loader and as we plan to removing the whole /addon-sdk folder from mozilla-central, we should make a copy of it in devtools folder.
While doing that, we could strip many features that DevTools don't use and have some performance win.
DAMP results (to come, I just pushed to try)
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=46cd72b4e7694034efd17b011bec92a1ba0ad831&newProject=try&newRevision=5c8abb2e79ed901a3dc4ca55cfb94b894becf76f&framework=1&showOnlyImportant=0

The removal of override and descriptor usages in the SDK, plus some random object creation savings allow to speed up toolbox opening on the inspector.
I see the open time go from 12170ms to 8526ms, (~30% win, but I only ran tests once).
But I see a constant object creation win from 131845 to 118849 objects (yes, we create more than 100k object when opening the toolbox. And that number is only for the parent process). Here we are ~9% win only.
Whiteboard: [nosdk]
Comment on attachment 8899814 [details]
Bug 1392602 - Simplify SDK loader to only match DevTools needs.

https://reviewboard.mozilla.org/r/171140/#review176516

::: devtools/shared/loader.js:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Can you use `hg cp` for this so we preserve change history?

::: devtools/shared/loader.js:12
(Diff revision 1)
> +const { loadSubScript } = Cc['@mozilla.org/moz/jssubscript-loader;1'].
> +                     getService(Ci.mozIJSSubScriptLoader);
> +const { notifyObservers } = Cc['@mozilla.org/observer-service;1'].
> +                        getService(Ci.nsIObserverService);

We should probably just use Services.obs and Services.scriptloader for these.

::: devtools/shared/loader.js:17
(Diff revision 1)
> +const { loadSubScript } = Cc['@mozilla.org/moz/jssubscript-loader;1'].
> +                     getService(Ci.mozIJSSubScriptLoader);
> +const { notifyObservers } = Cc['@mozilla.org/observer-service;1'].
> +                        getService(Ci.nsIObserverService);
> +const { XPCOMUtils } = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
> +const { NetUtil } = Cu.import("resource://gre/modules/NetUtil.jsm", {});

We should be able to make this a lazy import, now, if we switch to Services.io.newURI.

::: devtools/shared/loader.js:54
(Diff revision 1)
> +function parseStack(stack) {
> +  let lines = String(stack).split("\n");
> +  return lines.reduce(function(frames, line) {
> +    if (line) {
> +      let atIndex = line.indexOf("@");
> +      let columnIndex = line.lastIndexOf(":");
> +      let lineIndex = line.lastIndexOf(":", columnIndex - 1);
> +      let fileName = parseURI(line.slice(atIndex + 1, lineIndex));
> +      let lineNumber = parseInt(line.slice(lineIndex + 1, columnIndex));
> +      let columnNumber = parseInt(line.slice(columnIndex + 1));
> +      let name = line.slice(0, atIndex).split("(").shift();
> +      frames.unshift({
> +        fileName: fileName,
> +        name: name,
> +        lineNumber: lineNumber,
> +        columnNumber: columnNumber
> +      });
> +    }
> +    return frames;
> +  }, []);
> +}
> +Loader.parseStack = parseStack;
> +
> +function serializeStack(frames) {
> +  return frames.reduce(function(stack, frame) {
> +    return frame.name + "@" +
> +           frame.fileName + ":" +
> +           frame.lineNumber + ":" +
> +           frame.columnNumber + "\n" +
> +           stack;
> +  }, "");
> +}
> +Loader.serializeStack = serializeStack;

I'd suggest removing this crud, too. In my experience, it does more harm than good.

::: devtools/shared/loader.js:145
(Diff revision 1)
> +    principal: 'principal' in options ? options.principal : systemPrincipal,
> +    wantXrays: 'wantXrays' in options ? options.wantXrays : true,

I suspect these aren't useful anymore.

::: devtools/shared/loader.js:152
(Diff revision 1)
> +    wantGlobalProperties: 'wantGlobalProperties' in options ?
> +                          options.wantGlobalProperties : [],
> +    sandboxPrototype: 'prototype' in options ? options.prototype : {},
> +    invisibleToDebugger: 'invisibleToDebugger' in options ?
> +                         options.invisibleToDebugger : false,
> +    waiveIntereposition: !!options.waiveIntereposition

Nor this.

::: devtools/shared/loader.js:160
(Diff revision 1)
> +  delete sandbox.Iterator;
> +  delete sandbox.Components;

These deletions aren't necessary.

::: devtools/shared/loader.js:188
(Diff revision 1)
> +  }
> +  if (!version) {
> +    version = '1.8';
> +  }
> +
> +  return source ? Cu.evalInSandbox(source, sandbox, version, uri, line)

Can we get rid of the evalInSandbox variant? Or, really, just get rid of this function entirely and just use loadSubScript directly?

::: devtools/shared/loader.js:215
(Diff revision 1)
> +    Components: {
> +      configurable: true,
> +      get() {
> +        // Expose `Components` property to throw error on usage with
> +        // additional information
> +        throw new ReferenceError(COMPONENT_ERROR);
> +      }
> +    }

Can we get rid of this and just let `Components` stay undefined? It shouldn't be necessary for devtools code.

::: devtools/shared/loader.js:275
(Diff revision 1)
> +    let { message, fileName, lineNumber } = error;
> +    let stack = error.stack || Error().stack;
> +    let frames = parseStack(stack).filter(isntLoaderFrame);
> +    let toString = String(error);
> +    let file = sourceURI(fileName);
> +
> +    // Note that `String(error)` where error is from subscript loader does
> +    // not puts `:` after `"Error"` unlike regular errors thrown by JS code.
> +    // If there is a JS stack then this error has already been handled by an
> +    // inner module load.
> +    if (/^Error opening input stream/.test(String(error))) {
> +      let caller = frames.slice(0).pop();
> +      fileName = caller.fileName;
> +      lineNumber = caller.lineNumber;
> +      message = "Module `" + module.id + "` is not found at " + module.uri;
> +      toString = message;
> +    }
> +    // Workaround for a Bug 910653. Errors thrown by subscript loader
> +    // do not include `stack` field and above created error won't have
> +    // fileName or lineNumber of the module being loaded, so we ensure
> +    // it does.
> +    else if (frames[frames.length - 1].fileName !== file) {
> +      frames.push({ fileName: file, lineNumber: lineNumber, name: "" });
> +    }
> +
> +    let prototype = typeof(error) === "object" ? error.constructor.prototype :
> +                    Error.prototype;
> +
> +    throw Object.create(prototype, {
> +      message: { value: message, writable: true, configurable: true },
> +      fileName: { value: fileName, writable: true, configurable: true },
> +      lineNumber: { value: lineNumber, writable: true, configurable: true },
> +      stack: { value: serializeStack(frames), writable: true, configurable: true },
> +      toString: { value: () => toString, writable: true, configurable: true },
> +    });

Per comment above, can we get rid of this stack mangling gunk?

::: devtools/shared/loader.js:509
(Diff revision 1)
> +      // First attempt to load and parse json uri
> +      // ex: `test.json`
> +      // If that doesn't exist, check for `test.json.js`
> +      // for node parity
> +      try {
> +        data = JSON.parse(readURI(uri));

Is this used by devtools? It would be nice to get rid of the `readURI` stuff.

::: devtools/shared/loader.js:604
(Diff revision 1)
> +
> +const main = function main(loader, id) {
> +  // If no main entry provided, and native loader is used,
> +  // read the entry in the manifest
> +  let uri = resolveURI(id, loader.mapping);
> +  let module = loader.main = loader.modules[uri] = Module(id, uri);
> +  return loader.load(loader, module).exports;
> +}
> +Loader.main = main;

Is this used by devtools?
Status: NEW → ASSIGNED
Flags: qe-verify-
Priority: -- → P1
Target Milestone: --- → Firefox 57
(In reply to Kris Maglione [:kmag] from comment #3)
> Comment on attachment 8899814 [details]
> Bug 1392602 - Copy and simplify SDK loader to devtools.
> 
> https://reviewboard.mozilla.org/r/171140/#review176516
> 
> ::: devtools/shared/loader.js:1
> (Diff revision 1)
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> 
> Can you use `hg cp` for this so we preserve change history?

I'm using git/cinnabar, but I'll split this in two changesets.
It should fix that.

> > +Loader.serializeStack = serializeStack;
> 
> I'd suggest removing this crud, too. In my experience, it does more harm
> than good.

> Per comment above, can we get rid of this stack mangling gunk?

I'll look into this stack thing independently.

> 
> ::: devtools/shared/loader.js:509
> (Diff revision 1)
> > +      // First attempt to load and parse json uri
> > +      // ex: `test.json`
> > +      // If that doesn't exist, check for `test.json.js`
> > +      // for node parity
> > +      try {
> > +        data = JSON.parse(readURI(uri));
> 
> Is this used by devtools? It would be nice to get rid of the `readURI` stuff.

Yes, to load some data files like here:
http://searchfox.org/mozilla-central/source/devtools/server/actors/webaudio.js#25

> > +Loader.main = main;
> 
> Is this used by devtools?

no
(In reply to Kris Maglione [:kmag] from comment #3)
> Comment on attachment 8899814 [details]
> Bug 1392602 - Simplify SDK loader to only match DevTools needs.
> 
> https://reviewboard.mozilla.org/r/171140/#review176516
> 
> ::: devtools/shared/loader.js:1
> (Diff revision 1)
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> 
> Can you use `hg cp` for this so we preserve change history?
> 
> ::: devtools/shared/loader.js:12
> (Diff revision 1)
> > +const { loadSubScript } = Cc['@mozilla.org/moz/jssubscript-loader;1'].
> > +                     getService(Ci.mozIJSSubScriptLoader);
> > +const { notifyObservers } = Cc['@mozilla.org/observer-service;1'].
> > +                        getService(Ci.nsIObserverService);
> 
> We should probably just use Services.obs and Services.scriptloader for these.

I had to split this patch in pieces in order to narrow down this issue:
https://treeherder.mozilla.org/logviewer.html#?job_id=125561025&repo=try&lineNumber=7088
SecurityError: Failed to load worker script at "resource://devtools/client/shared/widgets/GraphsWorker.js"
Which related to this call:
http://searchfox.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#4933
  if (JS::DescribeScriptedCaller(aCx, &fileName)) {
Where `fileName` was "chrome://mochikit/content/browser-test.js -> resource://devtools/shared/worker/worker.js"
instead of just "resource://devtools/shared/worker/worker.js".
This was related to using `Services.scriptloader.loadSubscript` instead of `loadSubScript`.
I have no idea why it is any different, but I revert that back to use `loadSubScript`, so not change anything.
But I got rid of `evaluate` helper function and call `loadSubScript` directly from `load`.
I ran some more performances tests locally and see a 3-4% win. 
5941ms down to 5744ms to open the inspector.
And also from 121984 to down to 109063 js objects being created during toolbox opening.
Getting rid of override/descriptor helpers is the major win in object creation.
(In reply to Alexandre Poirot [:ochameau] from comment #20)
> Where `fileName` was "chrome://mochikit/content/browser-test.js ->
> resource://devtools/shared/worker/worker.js"
> instead of just "resource://devtools/shared/worker/worker.js".
> This was related to using `Services.scriptloader.loadSubscript` instead of
> `loadSubScript`.
> I have no idea why it is any different, but I revert that back to use
> `loadSubScript`, so not change anything.
> But I got rid of `evaluate` helper function and call `loadSubScript`
> directly from `load`.

Hrm. Apparently this is because browser-test.js replaces Services.scriptloader:

http://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/testing/mochitest/browser-test.js#169-186

We should probably just fix that rather than try to work around it by avoiding Services.scriptloader. Maybe it's about time to just fix bug 1356412.
bug 1356412 is one thing, but that hack in browser-test.js looks also very wrong :/
Depends on: 1394479
Attachment #8900735 - Flags: review?(jdescottes)
These patches still fail eslint, I'll address it after the review as adressing review comments are very likely going to conflict with eslint fixes!
Depends on: 1394804
Comment on attachment 8900735 [details]
Bug 1392602 - Copy SDK Loader to devtools.

https://reviewboard.mozilla.org/r/172186/#review179430

The sdk loader is still imported in devtools/client/shared/test/unit/test_undoStack.js

Can we find another name for loader.js ? We have devtools/shared/Loader.jsm and devtools/shared/loader.js. base-loader, toolkit-loader, sdk-loader?

::: devtools/shared/Loader.jsm:8
(Diff revision 3)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
>  /**
>   * Manages the addon-sdk loader instance used to load the developer tools.

Comment mentions the addon-sdk loader, should say loader.js (unless we agree on a better more specific name)

::: devtools/shared/builtin-modules.js:175
(Diff revision 3)
>  }
>  
>  // List of pseudo modules exposed to all devtools modules.
>  exports.modules = {
>    "Services": Object.create(Services),
>    "toolkit/loader": Loader,

We are still exporting this loader as "toolkit/loader". Related to my comments about naming, maybe the file should still be called toolkit-loader then? Make it easier to differenciate from the DevTools Loader when we discuss about it.

::: devtools/shared/loader.js:1
(Diff revision 3)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

It would really be nice to actually have a `hg copy` of the sdk loader. If you are unable to do it with the git setup, let me know, I can provide this first patch.
Attachment #8900735 - Flags: review?(jdescottes) → review+
Comment on attachment 8901825 [details]
Bug 1392602 - Remove module boilerplate from DevTools loader.

https://reviewboard.mozilla.org/r/173256/#review179434

::: devtools/shared/loader.js:183
(Diff revision 3)
>        });
>      }
>      return frames;
>    }, []);
>  }
> +// Used by devtools/server/actors/call-watcher.js

We should open a follow-up to move parseStack & serializeStack out of the loader if only used by call-watcher.js

::: devtools/shared/loader.js:1055
(Diff revision 3)
>    // cache may cause module identity problems in such cases.
>    let subject = { wrappedJSObject: loader.destructor };
>    notifyObservers(subject, 'sdk:loader:destroy', reason);
>  });
> +// Used by devtools/shared/Loader.jsm
>  Loader.unload = unload;

Just asking for consistency: any reason why this was kept on `Loader`, while for instance resolveURI was moved to `this` ?
Attachment #8901825 - Flags: review?(jdescottes) → review+
Comment on attachment 8901826 [details]
Bug 1392602 - Remove jetpack specifics from DevTools loader.

https://reviewboard.mozilla.org/r/173258/#review179436

::: devtools/shared/loader.js
(Diff revision 3)
>  
>      let requirement, uri;
>  
> -    // TODO should get native Firefox modules before doing node-style lookups
> +    if (modules[id]) {
> -    // to save on loading time
> -    if (isNative) {

isNative no longer used, but we can handle that in the eslint cleanup changeset you will be doing.
Attachment #8901826 - Flags: review?(jdescottes) → review+
Comment on attachment 8901827 [details]
Bug 1392602 - Stop freezing everything in DevTools loader.

https://reviewboard.mozilla.org/r/173260/#review179440
Attachment #8901827 - Flags: review?(jdescottes) → review+
Comment on attachment 8901828 [details]
Bug 1392602 - Remove `evaluate` function from DevTools loader.

https://reviewboard.mozilla.org/r/173262/#review179478

::: commit-message-cbb36:1
(Diff revision 3)
> +Bug 1392602 - Remove evaluate function. r=jdescottes

Message is too generic, mention the loader
Attachment #8901828 - Flags: review?(jdescottes) → review+
Comment on attachment 8901829 [details]
Bug 1392602 - Instanciate RegExp only once in DevTools loader.

https://reviewboard.mozilla.org/r/173264/#review179488

::: devtools/shared/loader.js:780
(Diff revision 3)
>  
>    return Object.create(null, returnObj);
>  };
>  this.Loader = Loader;
>  
> -var isSystemURI = uri => /^resource:\/\/(gre|devtools|testing-common)\//.test(uri);
> +var SystemRegExp = /^resource:\/\/(gre|devtools|testing-common)\//;

I would usually trust the engine to optimize this. I guess regexps can be harder to optimize for the browser since they have state?
Attachment #8901829 - Flags: review?(jdescottes) → review+
Comment on attachment 8901830 [details]
Bug 1392602 - Remove `override` helper in DevTools loader.

https://reviewboard.mozilla.org/r/173266/#review179492

::: commit-message-69084:1
(Diff revision 3)
> +Bug 1392602 - Get rid of override helper. r=jdescottes

commit message should mention the loader

::: devtools/shared/loader.js:633
(Diff revision 3)
> -    options.sharedGlobalBlocklist = options.sharedGlobalBlacklist;
> +  let resolve = Loader.resolve;
> +  if (!globals) {
> +    globals = {};
>    }
> -  let {
> -    modules, globals, resolve, paths, rootURI, manifest, isNative,
> +  if (!sharedGlobalBlocklist) {
> +    sharedGlobalBlocklist = ["sdk/indexed-db"];

We don't use sdk/indexed-db anymore, I think this can be removed. (as well as the other reference `wantGlobalProperties: module.id == "sdk/indexed-db" ? ["indexedDB"] : [],` in `function load`)
Attachment #8901830 - Flags: review?(jdescottes) → review+
Comment on attachment 8901831 [details]
Bug 1392602 - Remove `descriptor` helper in DevTools loader.

https://reviewboard.mozilla.org/r/173268/#review179502

::: commit-message-95cf7:1
(Diff revision 3)
> +Bug 1392602 - Get rid of descriptor helper. r=jdescottes

Commit message should mention the loader

::: devtools/shared/loader.js:174
(Diff revision 3)
>    // We expose set of properties defined by `CommonJS` specification via
>    // prototype of the sandbox. Also globals are deeper in the prototype
>    // chain so that each module has access to them as well.
> -  let descriptors = descriptor({
> -    require: require,
> -    module: module,
> +  let descriptors = {
> +    require: {
> +      configurable: true,

Why only configurable? I think this used to have writable and enumerable set to true as well. 

I'm fine with limiting to configurable if no code used to rely on writable/enumerable, but we should say why this has to be configurable in this case.
Attachment #8901831 - Flags: review?(jdescottes) → review+
Comment on attachment 8901832 [details]
Bug 1392602 - Fix eslint for the DevTools base loader.

https://reviewboard.mozilla.org/r/173270/#review179504
Attachment #8901832 - Flags: review?(jdescottes) → review+
Comment on attachment 8901833 [details]
Bug 1392602 - Stop using unnecessary const in Devtools loader.

https://reviewboard.mozilla.org/r/173272/#review179506

::: devtools/shared/loader.js
(Diff revision 3)
>      exports: { enumerable: true, writable: true, value: Object.create(null),
>                 configurable: true },
>      uri: { value: uri }
>    });
>  }
> -this.Module = Module;

In the same vain we can also get rid of this.Loader = Loader?
Attachment #8901833 - Flags: review?(jdescottes) → review+
Comment on attachment 8899814 [details]
Bug 1392602 - Simplify SDK loader to only match DevTools needs.

https://reviewboard.mozilla.org/r/171140/#review179508

Clearing the review flag for now. Technically I'm ok with change but before we land the file in DevTools, I want to do a post-eslint fixes review to check the quality/accuracy of comments in the file.

Thanks a lot for doing this :) (especially considering nobody thought about this issue for nosdk!)

::: devtools/shared/loader.js:133
(Diff revision 5)
>      wantGlobalProperties: 'wantGlobalProperties' in options ?
>                            options.wantGlobalProperties : [],
>      sandboxPrototype: 'prototype' in options ? options.prototype : {},
>      invisibleToDebugger: 'invisibleToDebugger' in options ?
>                           options.invisibleToDebugger : false,
> -    metadata: 'metadata' in options ? options.metadata : {},
> +    waiveIntereposition: false

So that's not directly related to your change but I seriously believe it is a long standing type of the SDK loader :)

This should be waiveInterposition and not waiveInter*e*position. Sandbox.cpp only knows about waiveInterposition.

::: devtools/shared/loader.js:203
(Diff revision 5)
>        name: module.uri,
>        prototype: Object.create(globals, descriptors),
> -      wantXrays: false,
>        wantGlobalProperties: module.id == "sdk/indexed-db" ? ["indexedDB"] : [],
>        invisibleToDebugger: loader.invisibleToDebugger,
>        metadata: {

metadata no longer consumed by Sandbox constructor

::: devtools/shared/loader.js:631
(Diff revision 5)
>    let sharedGlobalSandbox = Sandbox({
>      name: options.sandboxName || "Addon-SDK",
> -    wantXrays: false,
>      wantGlobalProperties: [],
>      invisibleToDebugger: options.invisibleToDebugger || false,
>      metadata: {

metadata is no longer consumed by the Sandbox constructor

::: devtools/shared/loader.js:632
(Diff revision 5)
>      name: options.sandboxName || "Addon-SDK",
> -    wantXrays: false,
>      wantGlobalProperties: [],
>      invisibleToDebugger: options.invisibleToDebugger || false,
>      metadata: {
>        addonID: options.noSandboxAddonId ? undefined : options.id,

We always pass noSandboxAddonId: true, it should removed from call sites if we remove "metadata"

::: devtools/shared/loader.js:668
(Diff revision 5)
>      id: { enumerable: false, value: options.id },
>      // Whether the modules loaded should be ignored by the debugger
>      invisibleToDebugger: { enumerable: false,
>                             value: options.invisibleToDebugger || false },
>      requireHook: { enumerable: false, value: options.requireHook },
>      loadModuleHook: { enumerable: false, value: options.loadModuleHook },

loadModuleHook is only used for hot reload which is going to go away with Bug 1392998 so it's probably safe to remove related code now.
Attachment #8899814 - Flags: review?(jdescottes)
(In reply to Julian Descottes [:jdescottes] from comment #49)
> Comment on attachment 8901827 [details]
> Bug 1392602 - Stop freezing everything in DevTools loader.
> 
> https://reviewboard.mozilla.org/r/173260/#review179440

I'm curious about this part. Why isn't freezing useful any longer?
(In reply to David Bruant from comment #68)
> (In reply to Julian Descottes [:jdescottes] from comment #49)
> > Comment on attachment 8901827 [details]
> > Bug 1392602 - Stop freezing everything in DevTools loader.
> > 
> > https://reviewboard.mozilla.org/r/173260/#review179440
> 
> I'm curious about this part. Why isn't freezing useful any longer?

It is moslty a matter of trust. Now the loader is going to be used only by DevTools.
So only one user. And we trust ourself in not hacking the loader by chanching it at runtime, nor workaround loader restrictions to win extra privileges.
(In reply to Julian Descottes [:jdescottes] from comment #46)
> The sdk loader is still imported in
> devtools/client/shared/test/unit/test_undoStack.js

Fixed.

> 
> Can we find another name for loader.js ? We have devtools/shared/Loader.jsm
> and devtools/shared/loader.js. base-loader, toolkit-loader, sdk-loader?

I went for base-loader.js, but we may followup and merge Loader.jsm with base-loader.js.

> We are still exporting this loader as "toolkit/loader". Related to my
> comments about naming, maybe the file should still be called toolkit-loader
> then? Make it easier to differenciate from the DevTools Loader when we
> discuss about it.

In the next changeset, I got rid of this magic.
Only callwatcher.js was using that, instead it will use Cu.import(base-loader.js)

> It would really be nice to actually have a `hg copy` of the sdk loader. If
> you are unable to do it with the git setup, let me know, I can provide this
> first patch.

You be fixed now.

(In reply to Julian Descottes [:jdescottes] from comment #47)
> We should open a follow-up to move parseStack & serializeStack out of the
> loader if only used by call-watcher.js

I'll open a followup to evaluate the usefulness of all that error/stack utils.
But parseStack and serializeStack are both still used by the loader itself.

> 
> ::: devtools/shared/loader.js:1055
> (Diff revision 3)
> >    // cache may cause module identity problems in such cases.
> >    let subject = { wrappedJSObject: loader.destructor };
> >    notifyObservers(subject, 'sdk:loader:destroy', reason);
> >  });
> > +// Used by devtools/shared/Loader.jsm
> >  Loader.unload = unload;
> 
> Just asking for consistency: any reason why this was kept on `Loader`, while
> for instance resolveURI was moved to `this` ?

No good reason. I removed all Loader\.xxxx = xxxx; occurences in this changeset.

(In reply to Julian Descottes [:jdescottes] from comment #48)
> isNative no longer used, but we can handle that in the eslint cleanup
> changeset you will be doing.

After all these patches applied, there is already no more occurences of isNative.

(In reply to Julian Descottes [:jdescottes] from comment #51)
> > -var isSystemURI = uri => /^resource:\/\/(gre|devtools|testing-common)\//.test(uri);
> > +var SystemRegExp = /^resource:\/\/(gre|devtools|testing-common)\//;
> 
> I would usually trust the engine to optimize this. I guess regexps can be
> harder to optimize for the browser since they have state?

You should not ;)
Here is was instanciating a new RegExp object, each time you call isSystemURI.
It is like calling: isSomething = () => { {}; return true; }, it will create the `{}` every time you call it.

(In reply to Julian Descottes [:jdescottes] from comment #52)
> ::: devtools/shared/loader.js:633
> (Diff revision 3)
> > -    options.sharedGlobalBlocklist = options.sharedGlobalBlacklist;
> > +  let resolve = Loader.resolve;
> > +  if (!globals) {
> > +    globals = {};
> >    }
> > -  let {
> > -    modules, globals, resolve, paths, rootURI, manifest, isNative,
> > +  if (!sharedGlobalBlocklist) {
> > +    sharedGlobalBlocklist = ["sdk/indexed-db"];
> 
> We don't use sdk/indexed-db anymore, I think this can be removed. (as well
> as the other reference `wantGlobalProperties: module.id == "sdk/indexed-db"
> ? ["indexedDB"] : [],` in `function load`)

Yes. Removed in this changeset.

(In reply to Julian Descottes [:jdescottes] from comment #53)
> Comment on attachment 8901831 [details]
> Bug 1392602 - Get rid of descriptor helper.
> 
> https://reviewboard.mozilla.org/r/173268/#review179502
> 
> ::: commit-message-95cf7:1
> (Diff revision 3)
> > +Bug 1392602 - Get rid of descriptor helper. r=jdescottes
> 
> Commit message should mention the loader
> 
> ::: devtools/shared/loader.js:174
> (Diff revision 3)
> >    // We expose set of properties defined by `CommonJS` specification via
> >    // prototype of the sandbox. Also globals are deeper in the prototype
> >    // chain so that each module has access to them as well.
> > -  let descriptors = descriptor({
> > -    require: require,
> > -    module: module,
> > +  let descriptors = {
> > +    require: {
> > +      configurable: true,
> 
> Why only configurable? I think this used to have writable and enumerable set
> to true as well. 
> 
> I'm fine with limiting to configurable if no code used to rely on
> writable/enumerable, but we should say why this has to be configurable in
> this case.

No good reason, I set them writable/enumerable.
Blocks: 1395741
Blocks: 1395754
Opened bug 1395754 to look into the error/stack helpers.

But there is a bunch of other possible followups:
* get rid of lazyRequire/lazyRequireModule if we really got rid of all SDK dependencies (these lazy helper are only used by SDK modules. DevTools has its own defined in builtin-modules.js)
* merge Loader.jsm and base-loader.js -*or*- try loading base-loader.js in the shared sandbox (it isn't crystal clear to me, but I'm wondering if it would prevent various cross compartment wrappers between base-loader.js which is a jsm in its own global versus all the modules in the shared sandbox compartment).
* we may be able to clean things even more:
  * remove getOwnIdentifiers (could use Object.getOwnPropertyDescriptors and ignore symbols),
  * consider useSharedGlobalSandbox always true and remove the other codepath
  * remove Sandbox helper method and inline it
  * make the loader create even less objects (getRequirements seems to create a bunch of useless objects)
(In reply to Alexandre Poirot [:ochameau] from comment #69)
> (In reply to David Bruant from comment #68)
> > I'm curious about this part. Why isn't freezing useful any longer?
> 
> It is moslty a matter of trust. Now the loader is going to be used only by
> DevTools.
> So only one user. And we trust ourself in not hacking the loader by
> chanching it at runtime, nor workaround loader restrictions to win extra
> privileges.

For what it's worth, I'm not convinced it was ever useful, even for the SDK
loader. SDK modules were never actually sandboxed, so they could use their
chrome privileges to do pretty much whatever they wanted, in the end. Even if
they *had* been sandboxed, they wouldn't have been able to modify the
prototypes of cross-origin loader objects they had access to, due to security
wrapper restrictions. And even ignoring that, had they been sandboxed, we
never should have given them access to cross-origin objects from the loader
compartment at all. The only objects they ever saw should have been created in
their compartment, or one of their same-origin module compartments.
Comment on attachment 8901825 [details]
Bug 1392602 - Remove module boilerplate from DevTools loader.

https://reviewboard.mozilla.org/r/173256/#review180370

::: devtools/server/actors/call-watcher.js:10
(Diff revision 6)
>  
>  /* global XPCNativeWrapper */
>  
>  const {Ci, Cu} = require("chrome");
>  const protocol = require("devtools/shared/protocol");
> -const {serializeStack, parseStack} = require("toolkit/loader");
> +const {serializeStack, parseStack} = Cu.import("resource://devtools/shared/loader.js", {});

resource://devtools/shared/loader.js -> resource://devtools/shared/base-loader.js
Comment on attachment 8901832 [details]
Bug 1392602 - Fix eslint for the DevTools base loader.

https://reviewboard.mozilla.org/r/173270/#review180398

One issue with "unload" that should be exported and a few nits. Looks good otherwise. Some parts are still a bit obscure but at least it looks like DevTools code now :)

::: devtools/shared/base-loader.js:9
(Diff revision 9)
>  
> -'use strict';
> +/* global __URI__ */
>  
> -this.EXPORTED_SYMBOLS = ["Loader", "resolveURI", "Module", "Require"]
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = ["Loader", "resolveURI", "Module", "Require"];

"unload" is not exported but is used by Loader.jsm

::: devtools/shared/base-loader.js:55
(Diff revision 9)
> -      let lineNumber = parseInt(line.slice(lineIndex + 1, columnIndex));
> -      let columnNumber = parseInt(line.slice(columnIndex + 1));
> +      let lineNumber = parseInt(line.slice(lineIndex + 1, columnIndex), 10);
> +      let columnNumber = parseInt(line.slice(columnIndex + 1), 10);

This eslint rule makes very little sense for the target browsers where this code has to run! (even for devtools-html projects)

::: devtools/shared/base-loader.js:115
(Diff revision 9)
>  //  - `name`: A string value which identifies the sandbox in about:memory. Will
>  //    throw exception if omitted.
>  // - `principal`: String URI or `nsIPrincipal` for the sandbox. Defaults to
>  //    system principal.
>  // - `prototype`: Ancestor for the sandbox that will be created. Defaults to
>  //    `{}`.
>  // - `sandbox`: A sandbox to share JS compartment with. If omitted new
>  //    compartment will be created.

principal and sandbox are not expected in the options object. 

missing doc for invisibleToDebugger

::: devtools/shared/base-loader.js:236
(Diff revision 9)
>        lineNumber = caller.lineNumber;
>        message = "Module `" + module.id + "` is not found at " + module.uri;
>        toString = message;
> -    }
> +    } else if (frames[frames.length - 1].fileName !== file) {
> -    // Workaround for a Bug 910653. Errors thrown by subscript loader
> +      // Workaround for a Bug 910653. Errors thrown by subscript loader
> -    // do not include `stack` field and above created error won't have
> +      // do not include `stack` field and above created error won"t have

won"t -> won't

::: devtools/shared/base-loader.js:551
(Diff revision 9)
>    });
>  }
>  
>  // Takes `loader`, and unload `reason` string and notifies all observers that
>  // they should cleanup after them-self.
> -function unload(loader, reason) {
> +function unload(loader, reason) { // eslint-disable-line no-unused-vars

I would prefer a single 
/* exported Loader, resolveURI, Module, Require, unload */ before our export statement.

::: devtools/shared/base-loader.js:560
(Diff revision 9)
> -  // asserted against loader.destructor or require('@loader/unload')
> +  // asserted against loader.destructor or require("@loader/unload")
>    // Note: We don not destroy loader's module cache or sandboxes map as
>    // some modules may do cleanup in subsequent turns of event loop. Destroying
>    // cache may cause module identity problems in such cases.
>    let subject = { wrappedJSObject: loader.destructor };
> -  notifyObservers(subject, 'sdk:loader:destroy', reason);
> +  notifyObservers(subject, "sdk:loader:destroy", reason);

We should think about using a different event at some point (one that doesn't mention sdk)

::: devtools/shared/base-loader.js:568
(Diff revision 9)
>  // Function makes new loader that can be used to load CommonJS modules.
>  // Loader takes following options:
>  // - `globals`: Optional map of globals, that all module scopes will inherit
>  //   from. Map is also exposed under `globals` property of the returned loader
>  //   so it can be extended further later. Defaults to `{}`.
>  // - `modules` Optional map of built-in module exports mapped by module id.

`modules` is no longer used. 

The complete list of options accepted and used by the loader is:

- globals
- id
- invisibleToDebugger
- paths
- requireHook
- sandboxName
- sandboxPrototype
- sharedGlobal

::: devtools/shared/base-loader.js:595
(Diff revision 9)
>  
>    // Define pseudo modules.
>    let modules = {
> -    '@loader/unload': destructor,
> -    '@loader/options': options,
> -    'chrome': { Cc: Cc, Ci: Ci, Cu: Cu, Cr: Cr, Cm: Cm,
> +    "@loader/unload": destructor,
> +    "@loader/options": options,
> +    "chrome": { Cc: Cc, Ci: Ci, Cu: Cu, Cr: Cr, Cm: Cm,

Can we `a: a -> a`
for all `Cc: Cc, Ci: Ci, Cu: Cu, Cr: Cr, Cm: Cm,`

::: devtools/shared/base-loader.js:597
(Diff revision 9)
>                  // `ChromeWorker` has to be inject in loader global scope.
>                  // It is done by bootstrap.js:loadSandbox for the SDK.

This whole comment is a bit confusing. Doesn't explain why it has to be inject(ed) in the global scope. + reference to SDK

::: devtools/shared/base-loader.js:623
(Diff revision 9)
>  
>      modules[uri] = module;
>    }
>  
>    // Create the unique sandbox we will be using for all modules,
>    // so that we prevent creating a new comportment per module.

comportment -> compartment ?

::: devtools/shared/base-loader.js:627
(Diff revision 9)
>    // Create the unique sandbox we will be using for all modules,
>    // so that we prevent creating a new comportment per module.
>    // The side effect is that all modules will share the same
>    // global objects.
>    let sharedGlobalSandbox = Sandbox({
>      name: options.sandboxName || "Addon-SDK",

"Addon-SDK" -> "DevTools"

::: devtools/shared/base-loader.js:643
(Diff revision 9)
>                              Object.getOwnPropertyDescriptor(globals, name));
> -  }
> +    }
> +  }
>  
>    // Loader object is just a representation of a environment
>    // state. We freeze it and mark make it's properties non-enumerable

"We freeze it and mark make it's properties [...]" 
-> "We mark its properties [...]"

::: devtools/shared/base-loader.js:667
(Diff revision 9)
> -var isJSONURI = uri => uri.endsWith('.json');
> -var isJSMURI = uri => uri.endsWith('.jsm');
> -var isJSURI = uri => uri.endsWith('.js');
> +var isJSONURI = uri => uri.endsWith(".json");
> +var isJSMURI = uri => uri.endsWith(".jsm");
> +var isJSURI = uri => uri.endsWith(".js");
>  var AbsoluteRegExp = /^(resource|chrome|file|jar):/;
>  var isAbsoluteURI = uri => AbsoluteRegExp.test(uri);
>  var isRelative = id => id.startsWith(".");

Move those helpers to the beginning of the file, use const instead of var
Comment on attachment 8899814 [details]
Bug 1392602 - Simplify SDK loader to only match DevTools needs.

https://reviewboard.mozilla.org/r/171140/#review180378

::: devtools/shared/base-loader.js:7
(Diff revision 8)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  'use strict';
>  
>  this.EXPORTED_SYMBOLS = ["Loader", "resolveURI", "Module", "Require"]

We don't export unload, but unload is used by Loader.jsm?
Attachment #8899814 - Flags: review?(jdescottes) → review+
Attachment #8901832 - Attachment is obsolete: true
Fixed most things in `Bug 1392602 - Simplify SDK loader to only match DevTools needs.` changeset, the rest in the eslint one.

> ::: devtools/shared/base-loader.js:560
> (Diff revision 9)
> > -  // asserted against loader.destructor or require('@loader/unload')
> > +  // asserted against loader.destructor or require("@loader/unload")
> >    // Note: We don not destroy loader's module cache or sandboxes map as
> >    // some modules may do cleanup in subsequent turns of event loop. Destroying
> >    // cache may cause module identity problems in such cases.
> >    let subject = { wrappedJSObject: loader.destructor };
> > -  notifyObservers(subject, 'sdk:loader:destroy', reason);
> > +  notifyObservers(subject, "sdk:loader:destroy", reason);
> 
> We should think about using a different event at some point (one that
> doesn't mention sdk)
> 

I'll add that to the followup list.

> The complete list of options accepted and used by the loader is:
> 
> - globals
> - id
> - invisibleToDebugger
> - paths
> - requireHook
> - sandboxName
> - sandboxPrototype
> - sharedGlobal

I commented them all and removed `id` which was specific to addons.

> ::: devtools/shared/base-loader.js:597
> (Diff revision 9)
> >                  // `ChromeWorker` has to be inject in loader global scope.
> >                  // It is done by bootstrap.js:loadSandbox for the SDK.
> 
> This whole comment is a bit confusing. Doesn't explain why it has to be
> inject(ed) in the global scope. + reference to SDK

I imagine it was for when loader.js was loaded in a custom sandbox or all-but-jsm,
you had to ensure exposing ChromeWorker. I thing we can remove this comment as ChromeWorker is available to JSMs.


> ::: devtools/shared/base-loader.js:667
> (Diff revision 9)
> > -var isJSONURI = uri => uri.endsWith('.json');
> > -var isJSMURI = uri => uri.endsWith('.jsm');
> > -var isJSURI = uri => uri.endsWith('.js');
> > +var isJSONURI = uri => uri.endsWith(".json");
> > +var isJSMURI = uri => uri.endsWith(".jsm");
> > +var isJSURI = uri => uri.endsWith(".js");
> >  var AbsoluteRegExp = /^(resource|chrome|file|jar):/;
> >  var isAbsoluteURI = uri => AbsoluteRegExp.test(uri);
> >  var isRelative = id => id.startsWith(".");
> 
> Move those helpers to the beginning of the file, use const instead of var

Moved them to top and used regular function,
it looks more natural.
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c8d0e1b96aec
Copy SDK Loader to devtools. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/1af00037c87d
Remove module boilerplate from DevTools loader. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/f6bcfdd2087d
Remove jetpack specifics from DevTools loader. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/5aeb0b74d00e
Stop freezing everything in DevTools loader. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/b550412100f2
Remove `evaluate` function from DevTools loader. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/0b60e81f892e
Instanciate RegExp only once in DevTools loader. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/8b9d7619aa05
Remove `override` helper in DevTools loader. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/27541e7dd9f1
Remove `descriptor` helper in DevTools loader. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/43ee58fa7537
Stop using unnecessary const in Devtools loader. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/ecaf010fd427
Simplify SDK loader to only match DevTools needs. r=jdescottes
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.