From 11836f7641748a08a1c0dcd8fd1dc859c0626c3f Mon Sep 17 00:00:00 2001 From: Gijs Kruitbosch Date: Tue, 21 Apr 2020 14:04:58 +0000 Subject: [PATCH] Bug 1631358 - remove traces of CPOWs from devtools, r=jdescottes,loganfsmyth Differential Revision: https://phabricator.services.mozilla.com/D71509 --- ...x_options_enable_serviceworkers_testing.js | 2 +- devtools/client/framework/test/head.js | 10 +- devtools/client/inspector/test/shared-head.js | 11 +- devtools/client/netmonitor/test/head.js | 11 +- devtools/client/responsive/browser/tunnel.js | 4 +- .../test/browser/_browser_console.ini | 2 - .../test/browser/browser_console_cpow.js | 136 ------------------ .../contributing/code-reviews-checklist.md | 2 +- devtools/docs/tests/writing-tests.md | 4 - devtools/server/actors/object.js | 7 - devtools/server/actors/object/stringifiers.js | 3 - devtools/server/tests/browser/head.js | 3 +- .../tests/xpcshell/test_objectgrips-21.js | 3 - devtools/shared/DevToolsUtils.js | 25 +--- devtools/shared/ThreadSafeDevToolsUtils.js | 8 -- 15 files changed, 13 insertions(+), 218 deletions(-) delete mode 100644 devtools/client/webconsole/test/browser/browser_console_cpow.js diff --git a/devtools/client/framework/test/browser_toolbox_options_enable_serviceworkers_testing.js b/devtools/client/framework/test/browser_toolbox_options_enable_serviceworkers_testing.js index cd6faf168f61..7b8d7c20c258 100644 --- a/devtools/client/framework/test/browser_toolbox_options_enable_serviceworkers_testing.js +++ b/devtools/client/framework/test/browser_toolbox_options_enable_serviceworkers_testing.js @@ -85,7 +85,7 @@ function toggleServiceWorkersTestingCheckbox() { function reload() { const promise = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser); - executeInContent("devtools:test:reload", {}, {}, false); + executeInContent("devtools:test:reload", {}, false); return promise; } diff --git a/devtools/client/framework/test/head.js b/devtools/client/framework/test/head.js index 6122ded80821..b16c65b1379b 100644 --- a/devtools/client/framework/test/head.js +++ b/devtools/client/framework/test/head.js @@ -76,22 +76,16 @@ function waitForContentMessage(name) { * @param {String} name The message name. Should be one of the messages defined * in doc_frame_script.js * @param {Object} data Optional data to send along - * @param {Object} objects Optional CPOW objects to send along * @param {Boolean} expectResponse If set to false, don't wait for a response * with the same name from the content script. Defaults to true. * @return {Promise} Resolves to the response data if a response is expected, * immediately resolves otherwise */ -function executeInContent( - name, - data = {}, - objects = {}, - expectResponse = true -) { +function executeInContent(name, data = {}, expectResponse = true) { info("Sending message " + name + " to content"); const mm = gBrowser.selectedBrowser.messageManager; - mm.sendAsyncMessage(name, data, objects); + mm.sendAsyncMessage(name, data); if (expectResponse) { return waitForContentMessage(name); } diff --git a/devtools/client/inspector/test/shared-head.js b/devtools/client/inspector/test/shared-head.js index 5031949f0902..c4e744bac53a 100644 --- a/devtools/client/inspector/test/shared-head.js +++ b/devtools/client/inspector/test/shared-head.js @@ -314,24 +314,17 @@ function waitForContentMessage(name) { * in doc_frame_script.js * @param {Object} data * Optional data to send along - * @param {Object} objects - * Optional CPOW objects to send along * @param {Boolean} expectResponse * If set to false, don't wait for a response with the same name * from the content script. Defaults to true. * @return {Promise} Resolves to the response data if a response is expected, * immediately resolves otherwise */ -function executeInContent( - name, - data = {}, - objects = {}, - expectResponse = true -) { +function executeInContent(name, data = {}, expectResponse = true) { info("Sending message " + name + " to content"); const mm = gBrowser.selectedBrowser.messageManager; - mm.sendAsyncMessage(name, data, objects); + mm.sendAsyncMessage(name, data); if (expectResponse) { return waitForContentMessage(name); } diff --git a/devtools/client/netmonitor/test/head.js b/devtools/client/netmonitor/test/head.js index ab66a87f8a97..1ab32a50f1e1 100644 --- a/devtools/client/netmonitor/test/head.js +++ b/devtools/client/netmonitor/test/head.js @@ -862,8 +862,6 @@ function performRequestsInContent(requests) { * shared/test/frame-script-utils.js * @param Object data * Optional data to send along - * @param Object objects - * Optional CPOW objects to send along * @param Boolean expectResponse * If set to false, don't wait for a response with the same name from the * content script. Defaults to true. @@ -872,15 +870,10 @@ function performRequestsInContent(requests) { * Resolves to the response data if a response is expected, immediately * resolves otherwise */ -function executeInContent( - name, - data = {}, - objects = {}, - expectResponse = true -) { +function executeInContent(name, data = {}, expectResponse = true) { const mm = gBrowser.selectedBrowser.messageManager; - mm.sendAsyncMessage(name, data, objects); + mm.sendAsyncMessage(name, data); if (expectResponse) { return waitForContentMessage(name); } diff --git a/devtools/client/responsive/browser/tunnel.js b/devtools/client/responsive/browser/tunnel.js index bd7faf273907..69fb94b956bb 100644 --- a/devtools/client/responsive/browser/tunnel.js +++ b/devtools/client/responsive/browser/tunnel.js @@ -678,9 +678,9 @@ MessageManagerTunnel.prototype = { debug(`${name} inner -> outer, sync: ${sync}`); if (sync) { - return this.outerChildMM.sendSyncMessage(name, data, objects, principal); + return this.outerChildMM.sendSyncMessage(name, data); } - this.outerChildMM.sendAsyncMessage(name, data, objects, principal); + this.outerChildMM.sendAsyncMessage(name, data); return undefined; }, diff --git a/devtools/client/webconsole/test/browser/_browser_console.ini b/devtools/client/webconsole/test/browser/_browser_console.ini index b79fb0263f6e..ff96738defd3 100644 --- a/devtools/client/webconsole/test/browser/_browser_console.ini +++ b/devtools/client/webconsole/test/browser/_browser_console.ini @@ -19,8 +19,6 @@ support-files = [browser_console.js] [browser_console_chrome_context_message.js] -[browser_console_cpow.js] -skip-if = !e10s # This test is only valid in e10s [browser_console_clear_cache.js] [browser_console_clear_closed_tab.js] [browser_console_clear_method.js] diff --git a/devtools/client/webconsole/test/browser/browser_console_cpow.js b/devtools/client/webconsole/test/browser/browser_console_cpow.js deleted file mode 100644 index b1db1fec0629..000000000000 --- a/devtools/client/webconsole/test/browser/browser_console_cpow.js +++ /dev/null @@ -1,136 +0,0 @@ -/* Any copyright is dedicated to the Public Domain. - * http://creativecommons.org/publicdomain/zero/1.0/ */ - -// Test the basic features of the Browser Console. - -"use strict"; - -const { - gDevToolsBrowser, -} = require("devtools/client/framework/devtools-browser"); - -add_task(async function() { - // Needed for the execute() function below - await pushPref("security.allow_parent_unrestricted_js_loads", true); - - await addTab("about:blank"); - - const hud = await BrowserConsoleManager.openBrowserConsoleOrFocus(); - - const { message, objectFront } = await obtainObjectWithCPOW(hud); - await testFrontEnd(message); - await testBackEnd(objectFront); - await clearOutput(hud, true); -}); - -async function obtainObjectWithCPOW(hud) { - info("Add a message listener, it will receive an object with a CPOW"); - await hud.evaluateJSAsync(` - Services.ppmm.addMessageListener("cpow", function listener(message) { - Services.ppmm.removeMessageListener("cpow", listener); - console.log(message.objects); - globalThis.result = message.objects; - }); - `); - - info("Open the browser content toolbox and send a message"); - const toolbox = await gDevToolsBrowser.openContentProcessToolbox(gBrowser); - const webConsoleFront = await toolbox.target.getFront("console"); - await webConsoleFront.evaluateJSAsync(` - let {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm"); - Services.cpmm.sendAsyncMessage("cpow", null, {cpow: {a:1}}); - `); - - info("Obtain the object with CPOW"); - const message = await waitFor(() => findMessage(hud, "cpow")); - const result = await hud.evaluateJSAsync("result"); - const objectFront = result.result; - - info("Cleanup"); - await hud.evaluateJSAsync("delete globalThis.result;"); - const onToolboxDestroyed = toolbox.once("destroyed"); - toolbox.topWindow.close(); - await onToolboxDestroyed; - - return { message, objectFront }; -} - -async function testFrontEnd(message) { - const oi = message.querySelector(".tree"); - const node = oi.querySelector(".tree-node"); - is(node.textContent, "Object { cpow: CPOW }", "Got an object with a CPOW"); - - expandObjectInspectorNode(node); - await waitFor(() => getObjectInspectorChildrenNodes(node).length > 0); - - const cpow = findObjectInspectorNodeChild(node, "cpow"); - const cpowObject = cpow.querySelector(".objectBox-object"); - is(cpowObject.textContent, "CPOW { }", "Got the CPOW"); - - expandObjectInspectorNode(cpow); - is(getObjectInspectorChildrenNodes(cpow).length, 0, "CPOW has no children"); -} - -async function testBackEnd(objectFront) { - // Check that inspecting an object with CPOW doesn't throw in the server. - // This would be done in a mochitest-chrome suite, but that doesn't run in - // e10s, so it's harder to get ahold of a CPOW. - - // Before the fix for Bug 1382833, this wouldn't resolve due to a CPOW error - // in the ObjectActor. - const prototypeAndProperties = await objectFront.getPrototypeAndProperties(); - - // The CPOW is in the "cpow" property. - const cpowFront = prototypeAndProperties.ownProperties.cpow.value; - - is(cpowFront.getGrip().class, "CPOW", "The CPOW grip has the right class."); - - // Check that various protocol request methods work for the CPOW. - let response = await cpowFront.getPrototypeAndProperties(); - is( - Reflect.ownKeys(response.ownProperties).length, - 0, - "No property was retrieved." - ); - is(response.ownSymbols.length, 0, "No symbol property was retrieved."); - is(response.prototype.type, "null", "The prototype is null."); - - response = await cpowFront.enumProperties({ ignoreIndexedProperties: true }); - let slice = await response.slice(0, response.count); - is( - Reflect.ownKeys(slice.ownProperties).length, - 0, - "No property was retrieved." - ); - - response = await cpowFront.enumProperties({}); - slice = await response.slice(0, response.count); - is( - Reflect.ownKeys(slice.ownProperties).length, - 0, - "No property was retrieved." - ); - - response = await cpowFront.getOwnPropertyNames(); - is(response.ownPropertyNames.length, 0, "No property was retrieved."); - - response = await cpowFront.getProperty("x"); - is(response.descriptor, undefined, "The property does not exist."); - - response = await cpowFront.enumSymbols(); - slice = await response.slice(0, response.count); - is(slice.ownSymbols.length, 0, "No symbol property was retrieved."); - - response = await cpowFront.getPrototype(); - is(response.prototype.type, "null", "The prototype is null."); - - response = await cpowFront.getDisplayString(); - is(response.displayString, "", "The CPOW stringifies to "); -} - -function findObjectInspectorNodeChild(node, nodeLabel) { - return getObjectInspectorChildrenNodes(node).find(child => { - const label = child.querySelector(".object-label"); - return label && label.textContent === nodeLabel; - }); -} diff --git a/devtools/docs/contributing/code-reviews-checklist.md b/devtools/docs/contributing/code-reviews-checklist.md index 37230af57919..4e3425c0f308 100644 --- a/devtools/docs/contributing/code-reviews-checklist.md +++ b/devtools/docs/contributing/code-reviews-checklist.md @@ -40,7 +40,7 @@ It can also be useful for patch authors: if the changes comply with these guidel * Test changes: * The feature or bug is [tested by new tests, or a modification of existing tests](../tests/writing-tests.md). * [Test logging](../tests/writing-tests.md#logs-and-comments) is sufficient to help investigating test failures/timeouts. - * [Test is e10s compliant](../tests/writing-tests.md#e10s-electrolysis) (no CPOWs, no content, etc…). + * [Test is e10s compliant](../tests/writing-tests.md#e10s-electrolysis) (doesn't try to access web content from the parent process, etc…). * Tests are [clean and maintainable](../tests/writing-tests.md#writing-clean-maintainable-test-code). * A try push has started (or even better, is green already). * User facing changes: diff --git a/devtools/docs/tests/writing-tests.md b/devtools/docs/tests/writing-tests.md index e75609e617df..f61df8938b37 100644 --- a/devtools/docs/tests/writing-tests.md +++ b/devtools/docs/tests/writing-tests.md @@ -120,10 +120,6 @@ You can learn more about E10S [from this blog post](https://timtaubert.de/blog/2 One of the direct consequences of E10S on tests is that you cannot retrieve and manipulate objects from the content page as you'd do without E10S. -Well this isn't entirely true, because with [cross-process object wrappers](https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Cross_Process_Object_Wrappers CPOWs) you can somehow access the page, get to DOM nodes, and read their attributes for instance, but a lot of other things you'd expect to work without E10S won't work exactly the same or at all. - -Using CPOWs is discouraged; they are only temporarily allowed in mochitests, and their use is forbidden in browser code. - So when creating a new test, if this test needs to access the content page in any way, you can use [the message manager](https://developer.mozilla.org/en-US/docs/The_message_manager) to communicate with a script loaded in the content process to do things for you instead of accessing objects in the page directly. You can use the helper `ContentTask.spawn()` for this. See [this list of DevTools tests that use that helper for examples](https://dxr.mozilla.org/mozilla-central/search?q=ContentTask.spawn%28+path%3Adevtools%2Fclient&redirect=false&case=false). diff --git a/devtools/server/actors/object.js b/devtools/server/actors/object.js index a985be761375..8efa0bfb49b9 100644 --- a/devtools/server/actors/object.js +++ b/devtools/server/actors/object.js @@ -135,13 +135,6 @@ const proto = { actor: this.actorID, }; - // Unsafe objects must be treated carefully. - if (DevToolsUtils.isCPOW(this.obj)) { - // Cross-process object wrappers can't be accessed. - g.class = "CPOW"; - return g; - } - const unwrapped = DevToolsUtils.unwrap(this.obj); if (unwrapped === undefined) { // Objects belonging to an invisible-to-debugger compartment might be proxies, diff --git a/devtools/server/actors/object/stringifiers.js b/devtools/server/actors/object/stringifiers.js index b144621e75a7..2b0fab0fe6d2 100644 --- a/devtools/server/actors/object/stringifiers.js +++ b/devtools/server/actors/object/stringifiers.js @@ -22,9 +22,6 @@ loader.lazyRequireGetter( */ function stringify(obj) { if (!DevToolsUtils.isSafeDebuggerObject(obj)) { - if (DevToolsUtils.isCPOW(obj)) { - return ""; - } const unwrapped = DevToolsUtils.unwrap(obj); if (unwrapped === undefined) { return ""; diff --git a/devtools/server/tests/browser/head.js b/devtools/server/tests/browser/head.js index 56d53d6e7f02..c581ddfb30b8 100644 --- a/devtools/server/tests/browser/head.js +++ b/devtools/server/tests/browser/head.js @@ -37,8 +37,7 @@ waitForExplicitFinish(); * @param {String} url The url to be loaded in the new tab * @return a promise that resolves to the new browser that the document * is loaded in. Note that we cannot return the document - * directly, since this would be a CPOW in the e10s case, - * and Promises cannot be resolved with CPOWs (see bug 1233497). + * directly, as we aren't able to access that in the parent. */ var addTab = async function(url) { info(`Adding a new tab with URL: ${url}`); diff --git a/devtools/server/tests/xpcshell/test_objectgrips-21.js b/devtools/server/tests/xpcshell/test_objectgrips-21.js index b414503eff6f..048f587e1d68 100644 --- a/devtools/server/tests/xpcshell/test_objectgrips-21.js +++ b/devtools/server/tests/xpcshell/test_objectgrips-21.js @@ -56,9 +56,6 @@ const defaults = { afterTest: "true == true", }; -// Obtaining a CPOW here does not seem possible, so the CPOW test is in -// devtools/client/webconsole/test/browser/browser_console.js - // The following tests use a system principal debuggee. const systemPrincipalTests = [ { diff --git a/devtools/shared/DevToolsUtils.js b/devtools/shared/DevToolsUtils.js index d6b40826cb15..74a4bd41e6bb 100644 --- a/devtools/shared/DevToolsUtils.js +++ b/devtools/shared/DevToolsUtils.js @@ -33,21 +33,6 @@ for (const key of Object.keys(ThreadSafeDevToolsUtils)) { exports[key] = ThreadSafeDevToolsUtils[key]; } -/** - * Helper for Cu.isCrossProcessWrapper that works with Debugger.Objects. - * This will always return false in workers (see the implementation in - * ThreadSafeDevToolsUtils.js). - * - * @param Debugger.Object debuggerObject - * @return bool - */ -exports.isCPOW = function(debuggerObject) { - try { - return Cu.isCrossProcessWrapper(debuggerObject.unsafeDereference()); - } catch (e) {} - return false; -}; - /** * Waits for the next tick in the event loop to execute a callback. */ @@ -182,7 +167,7 @@ exports.getProperty = function(object, key, invokeUnsafeGetters = false) { * objects belong to this case. * - Otherwise, if the debuggee doesn't subsume object's compartment, returns `null`. * - Otherwise, if the object belongs to an invisible-to-debugger compartment, - * returns `undefined`. Note CPOW objects belong to this case. + * returns `undefined`. * - Otherwise, returns the unwrapped object. */ exports.unwrap = function unwrap(obj) { @@ -194,7 +179,6 @@ exports.unwrap = function unwrap(obj) { // Attempt to unwrap via `obj.unwrap()`. Note that: // - This will return `null` if the debuggee does not subsume object's compartment. // - This will throw if the object belongs to an invisible-to-debugger compartment. - // This case includes CPOWs (see bug 1391449). // - This will return `obj` if there is no wrapper. let unwrapped; try { @@ -216,18 +200,13 @@ exports.unwrap = function unwrap(obj) { * Checks whether a debuggee object is safe. Unsafe objects may run proxy traps or throw * when using `proto`, `isExtensible`, `isFrozen` or `isSealed`. Note that safe objects * may still throw when calling `getOwnPropertyNames`, `getOwnPropertyDescriptor`, etc. - * Also note CPOW objects are considered to be unsafe, and DeadObject objects to be safe. + * Also note DeadObject objects are considered safe. * * @param obj Debugger.Object * The debuggee object to be checked. * @return boolean */ exports.isSafeDebuggerObject = function(obj) { - // CPOW usage is forbidden outside tests (bug 1465911) - if (exports.isCPOW(obj)) { - return false; - } - const unwrapped = exports.unwrap(obj); // Objects belonging to an invisible-to-debugger compartment might be proxies, diff --git a/devtools/shared/ThreadSafeDevToolsUtils.js b/devtools/shared/ThreadSafeDevToolsUtils.js index cc6300016797..6a102880fa5b 100644 --- a/devtools/shared/ThreadSafeDevToolsUtils.js +++ b/devtools/shared/ThreadSafeDevToolsUtils.js @@ -69,14 +69,6 @@ exports.values = function values(object) { return Object.keys(object).map(k => object[k]); }; -/** - * This is overridden in DevToolsUtils for the main thread, where we have the - * Cu object available. - */ -exports.isCPOW = function() { - return false; -}; - /** * Report that |who| threw an exception, |exception|. */