Closed Bug 1424721 Opened 5 years ago Closed 5 years ago

Can't copy longString nor store it as global variable

Categories

(DevTools :: Console, defect, P2)

Product:
DevTools
Component:
Console
Type:
defect

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: Oriol, Assigned: Oriol)

References

Details

Attachments

(1 file)

Bug 1424721 - Allow long strings and invisible-to-debugger objects to be stored as global variables.
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details 
Enter this to the console:

    "a".repeat(1e4);

Right-click the result and choose "Store as global variable" or "Copy object".

It doesn't happen because of

error occurred while processing 'evaluateJSAsync: TypeError: objActor.obj is undefined
Stack: evalWithDebugger@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/webconsole.js:1323:13
onEvaluateJS@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/webconsole.js:917:20
onEvaluateJSAsync@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/webconsole.js:888:20
onPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1788:15
receiveMessage@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:761:7
Line: 1323, column: 13
Invisible-to-debugger objects cannot be copied either.

    Components.utils.Sandbox(null, {invisibleToDebugger: true})
I added a `rawValue` method to all actors in order to have a common API to obtain the underlying JS value.
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
Sorry for the delay Oriol, this is a pretty busy week :)
Priority: -- → P2
Comment on attachment 8936519 [details]
Bug 1424721 - Allow long strings and invisible-to-debugger objects to be stored as global variables.

https://reviewboard.mozilla.org/r/207198/#review213484

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_store_as_global.js:22
(Diff revision 1)
> +  await Promise.all([ContentTask.spawn(gBrowser.selectedBrowser, null, () => {
> +    let {window} = content;
> +    let obj = Cu.Sandbox(Cu.getObjectPrincipal(window), {invisibleToDebugger: true});
> +    window.wrappedJSObject.invisibleToDebugger = obj;
> +    window.console.log("foo", obj);
> +  }), waitForMessage(hud, "foo")]);

this is a bit hard to read, could we split this ? 

const onFooLog = waitForMessage(hud, "foo")
ContentTask.spawn(gBrowser.selectedBrowser, null, () => {
    let {window} = content;
    let obj = Cu.Sandbox(Cu.getObjectPrincipal(window), {invisibleToDebugger: true});
    window.wrappedJSObject.invisibleToDebugger = obj;
    window.console.log("foo", obj);
});
await onFooLog;

Also, could we log something else than "foo" ("invisible" ?), i'm afraid of it clashing with the logs from the script in the test uri.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_store_as_global.js:29
(Diff revision 1)
> +  let messages = await waitFor(() => findMessages(hud, "foo"));
> +  is(messages.length, 5, "Five messages should have appeared");

i'd not wait for the invisible one in here. Since we already wait for invisible to be shown, we could get it directly, withoug using findMessages

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_store_as_global.js:67
(Diff revision 1)
> +  await storeAsVariable(hud, msgLongStr, "string");
> +  let equality = hud.jsterm.getInputValue() + " === longString";
> +  executedResult = await hud.jsterm.requestEvaluation(equality);
> +  is(executedResult.result, true, "Correct variable assigned into console.");

can we use the same pattern than for other tests ? Or change how we check equality in other tests to we have a consistent way of testing this ?

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_store_as_global.js:68
(Diff revision 1)
>    ok(executedResult.textContent.includes("{ baz: 1 }"),
>       "Correct variable assigned into console " + executedResult.textContent);
> +
> +  info("Check store as global variable is enabled for long strings");
> +  await storeAsVariable(hud, msgLongStr, "string");
> +  let equality = hud.jsterm.getInputValue() + " === longString";

could we usde window.longString so it's more obvious where it comes from ?

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_store_as_global.js:72
(Diff revision 1)
> +  info("Check store as global variable is enabled for invisible-to-debugger objects");
> +  await storeAsVariable(hud, msgInvisible, "object");
> +  equality = hud.jsterm.getInputValue() + " === invisibleToDebugger";
> +  executedResult = await hud.jsterm.requestEvaluation(equality);
> +  is(executedResult.result, true, "Correct variable assigned into console.");

same thing here

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_store_as_global.js:79
(Diff revision 1)
> +  equality = hud.jsterm.getInputValue() + " === invisibleToDebugger";
> +  executedResult = await hud.jsterm.requestEvaluation(equality);
> +  is(executedResult.result, true, "Correct variable assigned into console.");
>  });
>  
> -async function storeAsVariable(hud, element) {
> +let tempIdx = 0;

Could we pass an expectedIndex to the storeAsVariableFunction , and handle the current tempIdx when we call it, instead of having it here ? 
I think it would be cleaner
I'll go through the actor changes when i have time to watch it in a calm environment :D
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #5)
> Also, could we log something else than "foo" ("invisible" ?), i'm afraid of
> it clashing with the logs from the script in the test uri.

> can we use the same pattern than for other tests ?

I would prefer not to, because the console only shows a small part of the string and I want to test that the whole string was assigned to the global variable.

> Or change how we check equality in other tests to we have a consistent way of testing this ?

OK, will try this.

> Could we pass an expectedIndex to the storeAsVariableFunction, and handle
> the current tempIdx when we call it, instead of having it here ? 
> I think it would be cleaner

The reasoning was being able to reorder the tests without having to change the expected variable name
Comment on attachment 8936519 [details]
Bug 1424721 - Allow long strings and invisible-to-debugger objects to be stored as global variables.

https://reviewboard.mozilla.org/r/207198/#review213874

I have a minor comment, but this is looking good. Thanks Oriol !

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_store_as_global.js:53
(Diff revision 2)
> -  ok(executedResult.textContent.includes("{ baz: 1 }"),
> -     "Correct variable assigned into console " + executedResult.textContent);
> +  ContentTask.spawn(gBrowser.selectedBrowser, null, () => {
> +    let obj = Cu.Sandbox(Cu.getObjectPrincipal(content), {invisibleToDebugger: true});
> +    content.wrappedJSObject.invisibleToDebugger = obj;
> +    content.console.log("foo", obj);
> +  });
> +  let msgInvisible = (await waitForMessage(hud, "foo")).node;

we should create a promise for waitForMessage before firing the log, otherwise we might get a race condition.

let onMessageInvisible = waitForMessage(hud, "foo")
ContentTask …
let msgInvisible = (await onMessageInvisible).node;
Attachment #8936519 - Flags: review?(nchevobbe) → review+
Fixed!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/440c32f8f0a9
Allow long strings and invisible-to-debugger objects to be stored as global variables. r=nchevobbe
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/440c32f8f0a9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
I have reproduced this bug with Nightly 59.0a1 (2017-12-11) on Windows 10, 64 Bit!

This bug's fix is verified with latest Nightly!

Build ID   : 20180120100135
User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
QA Whiteboard: [bugday-20180117]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.