859170 - Trying to select [...] for a very long message printed to the web console will hang Firefox

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Jason Smith [:jsmith]]

Attachment: Test Case

STR:

1. Load the attached test case
2. Open up the web console
3. Select [...]

Expected:

I'd expect a truncated string within a dialog to appear with some ... to indicate the string is too long to display.

Actual:

Firefox will get hung at this point.

Comment 1

[Mihai Sucan [:msucan]]

Thanks for the bug report! Going to submit a patch ASAP.

Comment 2

[Mihai Sucan [:msucan]]

Attachment: proposed patch

This is the proposed patch. The problem seems to be related to the veeery slow rendering of big text nodes. This is why the entire Firefox UI hangs.

The approach for this fix is to limit the text node length for strings coming from the LongStringActor. The constant I picked is based on my system: I picked a value that is rendered instantly. I did not limit the length of strings at the protocol level - if you think we should, please let me know.

The test uses the new waitForMessages() helper, with improvements. I expect I will continue to improve that, on a case-by-case basis (whenever I need it).

Try push: https://tbpl.mozilla.org/?tree=Try&rev=1dcaa73b3baa

Jason, Panos: do you believe we should make a patch for Aurora as well? This doesn't seem to be a hang that users would encounter often.

Thank you!

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 735951 [details] [diff] [review]
proposed patch

Review of attachment 735951 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Mihai Sucan [:msucan] from comment #2)
> Jason, Panos: do you believe we should make a patch for Aurora as well? This
> doesn't seem to be a hang that users would encounter often.

I don't see how we could make a useful fix without a new warning message and Aurora is string-frozen.

Comment 4

[Mihai Sucan [:msucan]]

Attachment: test fixes

Previous try push failed. 500000 chars in a text node is too much for debug builds on the try servers - had failures.

Lowered to 200000 chars and made other minor test changes and now I had a green try push:

https://tbpl.mozilla.org/?tree=Try&rev=1bd2a9d431d4

Thanks for the review. I intend to land this patch tomorrow, once bug 859858 reaches fx-team from inbound.

Comment 5

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #3)
> Comment on attachment 735951 [details] [diff] [review]
> proposed patch
> 
> Review of attachment 735951 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Mihai Sucan [:msucan] from comment #2)
> > Jason, Panos: do you believe we should make a patch for Aurora as well? This
> > doesn't seem to be a hang that users would encounter often.
> 
> I don't see how we could make a useful fix without a new warning message and
> Aurora is string-frozen.

If a patch for aurora is needed we can simply limit how much we display from the long string (as we do now), and have no additional warning.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Mihai Sucan [:msucan] from comment #5)
> If a patch for aurora is needed we can simply limit how much we display from
> the long string (as we do now), and have no additional warning.

I expect this to result in new bugs filed: "console doesn't display long strings correctly". IMHO behavior that could be interpreted to indicate data corruption should be avoided.

Comment 7

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #6)
> (In reply to Mihai Sucan [:msucan] from comment #5)
> > If a patch for aurora is needed we can simply limit how much we display from
> > the long string (as we do now), and have no additional warning.
> 
> I expect this to result in new bugs filed: "console doesn't display long
> strings correctly". IMHO behavior that could be interpreted to indicate data
> corruption should be avoided.

For Aurora and Beta one would prefer such data corruption over a complete browser freeze that, depending on the string length, can result in a forced kill, which in turn results in more data loss.

Comment 8

[Mihai Sucan [:msucan]]

Landed in fx-team:

https://hg.mozilla.org/integration/fx-team/rev/501a15ed1e2b

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/501a15ed1e2b