Closed Bug 1310630 Opened 8 years ago Closed 6 years ago

Console should allow to get the full text of a longString

Categories

(DevTools :: Console, defect, P1)

defect

Tracking

(firefox49 unaffected, firefox50 unaffected, firefox51 unaffected, firefox52 disabled, firefox-esr52 disabled, firefox-esr60 wontfix, firefox53 disabled, firefox54 disabled, firefox56 disabled, firefox57 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- disabled
firefox-esr52 --- disabled
firefox-esr60 --- wontfix
firefox53 --- disabled
firefox54 --- disabled
firefox56 --- disabled
firefox57 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: marco, Assigned: nchevobbe)

References

Details

(Keywords: regression, Whiteboard: [newconsole-mvp])

Attachments

(1 file, 1 obsolete file)

fetch('http://ftp.mozilla.org/pub/firefox/nightly/2016/10/2016-10-01-03-04-30-mozilla-central/')
.then(response => response.text())
.then(data => {
  console.log(data);
})

This snippet used to work (data was not empty) before bug 1304178, doesn't work anymore now (data is empty).

Setting `devtools.webconsole.new-frontend-enabled` to false fixes the issue.

 5:43.79 INFO: Last good revision: 37f78aca862224d7151c0fcae1ed8373fe11c83b
 5:43.79 INFO: First bad revision: a5510966f80b9b2f5abf59ab32cf4c92d66c60de
 5:43.79 INFO: Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=37f78aca862224d7151c0fcae1ed8373fe11c83b&tochange=a5510966f80b9b2f5abf59ab32cf4c92d66c60de

 5:44.88 INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1304178
The results are actually accessible from JS, so it's just console.log which is failing.
Component: DOM → Developer Tools: Console
Product: Core → Firefox
Summary: fetch returning an empty body → console.log printing an empty body after fetch
Thanks for filing. The behavior of the old console matches Chrome as well, so we should fix this before turning it on in Dev Edition.
Assignee: nobody → chevobbe.nicolas
This is due to LongString object not being handled by Reps, and will be fixed when Bug 1302982 lands
Depends on: 1302982
Priority: -- → P2
Comment on attachment 8804383 [details]
Bug 1310630 - Add Rep for LongString. ;

https://reviewboard.mozilla.org/r/88384/#review88448

Please see my inline comment.

Honza

::: devtools/client/shared/components/reps/long-string.js:51
(Diff revision 2)
> +        config.style = style;
> +      }
> +
> +      let croppedString = cropLimit && !(member && member.open)
> +        ? initial.substring(0, cropLimit)
> +        : initial;

Shouldn't we process the string by cropString to make sure e.g. any non-pritable characters are replaced?

Also, if the string is resolved and `_fullText` field available on the grip, shouldn't we use it?

This is the place where `_fullText` is set
https://dxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/client.js#631
Attachment #8804383 - Flags: review?(odvarko)
Something we must have in mind too : in the old console, clicking on the "[...]" part at the end of the truncated string show the full content of the long string, so we should have the same behavior in the new one.
BTW, I mixed the bug, I wanted to attach this patch for Bug 1302982 , I might push the next patch there, and handle the "retrieve the full content when clicking on the ellipsis" here.
> Also, if the string is resolved and `_fullText` field available on the grip, shouldn't we use it?

From what I understand, _fullText is not directly in the grip object (and I guess it would be hard to have since it's a promise), but could be passed as a property to the Rep.
Now, I wonder if we couldn't have the fullText in the `member` object, or maybe replace it.
The member object seems to be set in the DOM Panel when clicking on a truncated string, so maybe it'd make sense to retrieve the full text at that moment.
Attachment #8804383 - Attachment is obsolete: true
Attachment #8807229 - Flags: review?(odvarko)
Attachment #8807229 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Comment on attachment 8807229 [details]
Bug 1310630 - Pass a createLongStringClient prop to the ObjectInspector; .

https://reviewboard.mozilla.org/r/90454/#review91554

Looks good, R+ assuming Try is green.
Attachment #8807229 - Flags: review+
Re-purposing this as Bug 1302982 landed and thus the console isn't broken anymore when logging a longString
Summary: console.log printing an empty body after fetch → Console should allow to get the full text of a longString
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #12)
> Re-purposing this as Bug 1302982 landed and thus the console isn't broken
> anymore when logging a longString

Nick, are the status flags still valid on this bug? It's showing up in regression triage.
Flags: needinfo?(chevobbe.nicolas)
I don't really know how regression flag works, but it can be seen as a regression since it works in the old console but not in the new one. 
The only thing that could make this not a regression is because the new console is preffed on only in Nightly, and this bug is a blocker for the bug where we plan to activate it everywhere (Bug 1308219).

Feel free to remove the flag if it makes sense to you.
Flags: needinfo?(chevobbe.nicolas)
We should still track this as a regression for whatever release eventually ships the new debugger. That said, setting the status for 52 to disabled since this is still Nightly-only.
No longer blocks: enable-new-console
In Bug 1358937 we will be able to display exceptionMessage sent as a longString in the console. This text will only be an excerpt of the full message since we'll only display the `initial` property and an ellipsis. We should also provide a way to "expand" the exception message to have its full content.
According to Bug 1397425, new frontend is riding with 57. Nicolas, are you still planning on working on this?
Flags: needinfo?(nchevobbe)
I am, let's add it to our tracking list
Flags: needinfo?(nchevobbe)
Whiteboard: [reserve-console-html]
Iteration: --- → 57.3 - Sep 19
Flags: qe-verify?
Priority: P2 → P1
Iteration: 57.3 - Sep 19 → ---
See Also: → 968976
Blocks: 1403448
Priority: P1 → P3
Assignee: nchevobbe → nobody
Status: ASSIGNED → NEW
Blocks: 1406192
Priority: P3 → P2
Whiteboard: [reserve-console-html] → [newconsole-mvp]
Flags: qe-verify?
Blocks: 1336097
Depends on: 1452566
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8807229 [details]
Bug 1310630 - Pass a createLongStringClient prop to the ObjectInspector; .

https://reviewboard.mozilla.org/r/90454/#review244284
Attachment #8807229 - Flags: review?(bgrinstead) → review+
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b0d116ec4546
Pass a createLongStringClient prop to the ObjectInspector; r=bgrins,Honza.
https://hg.mozilla.org/mozilla-central/rev/b0d116ec4546
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools

Coming here from https://bugzilla.mozilla.org/show_bug.cgi?id=1418382

What I understand from this issue, is that the problem should be solved 4 years ago, but it is still present in the current FF Developer (v. 102.0b9) & Stable (v. 101.0.1) releases.

Still truncated log/error messages, without possibility to expand them. Example:

'Uncaught TypeError: can't access property "currentSrc", $(...).find(...)[0] is undefined'

Should I create a new issue for this?

(In reply to tf from comment #25)

Coming here from https://bugzilla.mozilla.org/show_bug.cgi?id=1418382

What I understand from this issue, is that the problem should be solved 4 years ago, but it is still present in the current FF Developer (v. 102.0b9) & Stable (v. 101.0.1) releases.

Still truncated log/error messages, without possibility to expand them. Example:

'Uncaught TypeError: can't access property "currentSrc", $(...).find(...)[0] is undefined'

Should I create a new issue for this?

This looks different. Here it's probably the error message that is getting altered. Would you have a test page showing this issue?

Hi Nicolas, thank you so much for your fast response! Sure, take a look at the console on this pen: https://codepen.io/thomas-frobieter/pen/mdXYoew

I got a 'Uncaught TypeError: $(...).find(...)[0] is undefined'

(In reply to tf from comment #27)

Hi Nicolas, thank you so much for your fast response! Sure, take a look at the console on this pen: https://codepen.io/thomas-frobieter/pen/mdXYoew

I got a 'Uncaught TypeError: $(...).find(...)[0] is undefined'

Thanks for the link
So yeah, it's not related to the console: if you do the following:

try {
    let pictureCurrentSource = $(this).find('.field--name-field-coverimage img:first')[0].currentSrc;
} catch(e) {
    alert(e.message)
}

The text in the alert will contain the ...
I think it may be because the engine doesn't keep the argument of the function calls around, but I'm not sure.
Arai, is it worth opening a bug for this?

Flags: needinfo?(arai.unmht)

In short, if we keep the current error reporting approach, there's not much benefit for supporting parameters.
If there's much demand for showing the value, we could look into modifying the error reporting approach.

Here's the details:

The $(...).find(...)[0] string is generated by bytecode decompiler, that doesn't handle parameters and always use (...) or ().

https://searchfox.org/mozilla-central/rev/230a641415c4212fe719279263e8ddf2a411aff1/js/src/vm/BytecodeUtil.cpp#1803,1831,1956-1961

bool ExpressionDecompiler::decompilePC(jsbytecode* pc, uint8_t defIndex) {
...
  switch (op) {
...
    case JSOp::Call:
    case JSOp::CallIgnoresRv:
    case JSOp::CallIter: {
      uint16_t argc = GET_ARGC(pc);
      return decompilePCForStackOperand(pc, -int32_t(argc + 2)) &&
             write(argc ? "(...)" : "()");

The purpose of decompiler is to generate the codelet that returned the unexpected value (here, the undefined comes from the 0-th element of the value returned by find function), as a supplemental information for the error. The codelet does not represent the runtime value, but always the source code structure (or internal semantics of the source code if the source code is desugared into simpler bytecode sequence).

So, if the corresponding value comes from the code find('.field--name-field-coverimage img:first')[0], the codelet inside the error becomes find('.field--name-field-coverimage img:first')[0], but that's also available in the source pointed by the error, and IMO doesn't add much information.

On the other hand, if the code is find(query)[i], the codelet inside the error becomes find(query)[i], not find("<the content of query variable>")[0].
So, if the actual case that people want to see the value is this case, the current bytecode decompiler approach doesn't help.

If showing the actual value instead of (or in addition to) source code has more demand and more benefit, we could look into modifying the error reporting and integrate it into bytecode decompiler. for example, let it generate find(query /* = "<the content of query variable>" */)[i /* = 0 */] string.

The another possibility I can think of is to integrate the error reporting more with debugger, instead of just using string-based error message,
but if it's inside the debugger's context, the value is available by pausing at the exception and inspecting the variable.

Then, I want to know what the actual context the content of the string become necessary is.
(assuming the code above is just a minimal/simplified example)
If there's another and better way to inspect the value (such as, using debugger), that should be the way to go, instead of looking into error reporting.

Anyway, if you want to file a bug, please file under bug 622261.

Flags: needinfo?(arai.unmht)
You need to log in before you can comment on or make changes to this bug.