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)
DevTools
General
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
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.
Updated•7 years ago
|
Whiteboard: [nosdk]
Comment 3•7 years ago
|
||
mozreview-review |
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?
Assignee | ||
Comment 4•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=450c8d4d229626e3cb6d0d6ce0b1ee79e540e32c
Updated•7 years ago
|
Status: NEW → ASSIGNED
Flags: qe-verify-
Priority: -- → P1
Target Milestone: --- → Firefox 57
Assignee | ||
Comment 5•7 years ago
|
||
So this is the opposite of bug 1393086. I tend to see an improvement with local custom tests. But DAMP says there is no win at all: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=7e70aa85947eedfac3dd2223f7efc3225c0a0696&newProject=try&newRevision=07988c3e00e5797c935c911905b081f75918b92d&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1 That, even if I make DAMP toggle the toolbox only once: https://hg.mozilla.org/try/rev/1ec7988042a98e7e8410b4fc50cdee7481592152
Assignee | ||
Comment 6•7 years ago
|
||
(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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•7 years ago
|
||
(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`.
Assignee | ||
Comment 21•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 33•7 years ago
|
||
(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.
Assignee | ||
Comment 34•7 years ago
|
||
bug 1356412 is one thing, but that hack in browser-test.js looks also very wrong :/
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8900735 -
Flags: review?(jdescottes)
Assignee | ||
Comment 45•7 years ago
|
||
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!
Comment 46•7 years ago
|
||
mozreview-review |
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 47•7 years ago
|
||
mozreview-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 48•7 years ago
|
||
mozreview-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 49•7 years ago
|
||
mozreview-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 50•7 years ago
|
||
mozreview-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 51•7 years ago
|
||
mozreview-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 52•7 years ago
|
||
mozreview-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 53•7 years ago
|
||
mozreview-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 54•7 years ago
|
||
mozreview-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 55•7 years ago
|
||
mozreview-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 56•7 years ago
|
||
mozreview-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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 68•7 years ago
|
||
(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?
Assignee | ||
Comment 69•7 years ago
|
||
(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.
Assignee | ||
Comment 70•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 93•7 years ago
|
||
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)
Comment 94•7 years ago
|
||
(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 95•7 years ago
|
||
mozreview-review |
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 96•7 years ago
|
||
Try with the fix above: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3837c1629a2588e4b58f769959000f5223bff46
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 129•7 years ago
|
||
mozreview-review |
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 130•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8901832 -
Attachment is obsolete: true
Assignee | ||
Comment 141•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 161•7 years ago
|
||
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
Comment 162•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c8d0e1b96aec https://hg.mozilla.org/mozilla-central/rev/1af00037c87d https://hg.mozilla.org/mozilla-central/rev/f6bcfdd2087d https://hg.mozilla.org/mozilla-central/rev/5aeb0b74d00e https://hg.mozilla.org/mozilla-central/rev/b550412100f2 https://hg.mozilla.org/mozilla-central/rev/0b60e81f892e https://hg.mozilla.org/mozilla-central/rev/8b9d7619aa05 https://hg.mozilla.org/mozilla-central/rev/27541e7dd9f1 https://hg.mozilla.org/mozilla-central/rev/43ee58fa7537 https://hg.mozilla.org/mozilla-central/rev/ecaf010fd427
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•