Closed Bug 1779173 Opened 2 years ago Closed 2 years ago

Debugger freezes when viewing very long single-line JS file (3MB)

Categories

(DevTools :: Debugger, defect, P2)

Firefox 102
defect

Tracking

(firefox-esr91 unaffected, firefox-esr102 verified, firefox102 wontfix, firefox103 wontfix, firefox104 verified, firefox105 verified)

VERIFIED FIXED
105 Branch
Tracking Status
firefox-esr91 --- unaffected
firefox-esr102 --- verified
firefox102 --- wontfix
firefox103 --- wontfix
firefox104 --- verified
firefox105 --- verified

People

(Reporter: danweiss, Assigned: nchevobbe)

References

(Blocks 2 open bugs, Regression)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Firefox/102.0

Steps to reproduce:

These are steps I found that can trigger the bug:

Have your browser window be at least 1224 pixels wide
Open a stream on twitch.tv where there is video playing and chatting
Press F12 to open developer tools
Go to Inspector.
In search HTML, look for "showmore"
Should see <button class="ScCoreLink-sc-udwpw5-0 gBLUEB tw-link"..., with an "event" button.
Click on the "event" button
For the "onClick" row, click on the 'View In Debugger' button (it's a curved arrow pointing to two lines, it's to the left of the words "react" "bubbling")
It has switched over to the Debugger, now you are looking at a single line of javascript that is over 3MB large, and this is badly bogging down Firefox.

This seems to be a regression, as Firefox ESR 91 does not have this problem. I suspect this may have to do with the process isolation feature introduced around Firefox 100.

Actual results:

Firefox's "Javascript Debugger" in Web Developer Tools badly bogs down on a 3MB single line javascript file, taking over 1 minute before it resumes. Worse if there is other things happening in the background, such as video or chat.

Expected results:

The Debugger UI should have remained responsive and not froze.

The Bugbug bot thinks this bug should belong to the 'DevTools::General' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Component: Untriaged → General
Product: Firefox → DevTools
Component: General → Debugger
Summary: Web Developer Tools "Debugger" freezes when viewing very long single-line JS file → Debugger freezes when viewing very long single-line JS file (3MB)

For reference, this is the particular .js file that I encountered the issue on: https://static.twitchcdn.net/assets/core-58284c7f5f945e992e20.js . The file is 2,969,556 bytes large, and contains some Unicode characters.

In case that file goes missing, I made an archival copy at https://www.dwedit.org/files/.bugzilla/core-58284c7f5f945e992e20.7z (7-zip compressed)

When I view the .js file in the browser directly then use "View Source" view-source:https://static.twitchcdn.net/assets/core-58284c7f5f945e992e20.js, I also see a huge single line. If you scroll, Firefox shows blank space, then 2 seconds later, the text appears. However, the browser does not freeze when waiting for the text to appear, it remains responsive. As a side note, text at the end of the file has incorrect character spacing and looks weird, but that's a different bug.

Sounds similar to Bug 1778758. Surprising to see that it is a regression though. Will run mozregression.

As far as I can tell, this regressed in the following push: https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=6f0a8dddad511ef68fae3a4ab07cc9336516cc3a
(that push was probably wrong, so for the record, mozregression stopped at https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=73b74d9be741b4bc64964d2cf8a72a995badfb09&tochange=63ef5a7d2b1051617b4549384022cd9889d512da for me)

One notable difference is that before the content was displayed without any syntax highlighting, but relatively quickly. Now we seem to have syntax highlighting immediately, but the browser freezes.

Actually a regression from Bug 1754407, specifically from the last patch in the queue: https://hg.mozilla.org/integration/autoland/rev/dc88906824026db14115269cdc9f35f0ed3edbc6

Status: UNCONFIRMED → NEW
Ever confirmed: true
Regressed by: 1754407

From the other bug: profile at https://share.firefox.dev/3atrL2o

We should bail out of highlighting for large files, there seems to be a mechanism in CodeMirror to do that: https://searchfox.org/mozilla-central/rev/a352cc827575823676717d53766c39710b54201a/devtools/client/shared/sourceeditor/editor.js#161 we should check if this works

Flags: needinfo?(nchevobbe)
Severity: -- → S3
Priority: -- → P2

Not much progress yet, but I can confirm the main difference between before and after the regressing changeset is that before we never used to set the editor mode to "javascript" for those long source files, whereas we do now.

The maxHighlightLength seems to be working as intended: if I evaluate the following in the console eval(("x= 2;".repeat(210)) + "debugger;" );
it pauses the debugger, and I can see that the debugger statement isn't highlighted.

Flags: needinfo?(nchevobbe)

:ochameau, since you are the author of the regressor, bug 1754407, could you take a look?
For more information, please visit auto_nag documentation.

Flags: needinfo?(poirot.alex)

Will investigate today, we could force the mode to text for very big files (which seemed to be the behavior earlier)

Flags: needinfo?(poirot.alex) → needinfo?(jdescottes)

Hm, it seems like that's not an ideal fix -- I know that the Chromium version of the debugger (which, I understand is not the same; but I think it still uses CodeMirror) doesn't choke on syntax highlighting like the current version of the FF debugger.

(In reply to Brooke Baer [:brookerose1312] from comment #13)

Hm, it seems like that's not an ideal fix -- I know that the Chromium version of the debugger (which, I understand is not the same; but I think it still uses CodeMirror) doesn't choke on syntax highlighting like the current version of the FF debugger.

I agree, but it's the fastest to recover from the regression. Once that's done we can look into upgrading codemirror and trying to address the performance issue.

Flags: needinfo?(jdescottes)

Sounds good -- this has been blocking me using Firefox at work so that should hopefully help!

Disabling syntax highlighting does fix the issue, so this is not caused by something else (e.g. column breakpoints).

I need to check that again with a fresh brain, but I think this should be correct and seems to fix the perf issue:

  function countColumn(string, end, tabSize, startIndex, startValue) {
    // [...]
    for (var i = startIndex || 0, n = startValue || 0;;) {
      // slicing the string before checking the tab char index
      var nextTab = string.substring(i, end).indexOf("\t");
      if (nextTab < 0) { return n + (end - i) }
      n += nextTab;
      n += tabSize - (n % tabSize);
      i = nextTab + 1;
    }
  }

the idea is that most of the time was spent in indexOf, which makes sens as the string we're looking at is huge.
Getting just the token that needs to be checked makes it faster

Set release status flags based on info from the regressing bug 1754407

I tried to reproduce the issue in CodeMirror with the test case from Bug 1778758 , building this small app https://fx-devtools-codemirror5-long-lines.glitch.me/ that uses the exact same codeMirror bundle we're using in DevTools

Clicking the "Load big single-line file" button does fetch the single-line amo js file and put it in a codeMirror instance.
Unlike in DevTools, this happens quite fast:

we must be doing something wrong here, maybe calling something too many times? I'll continue to investigate

This seems to be the culprit: https://searchfox.org/mozilla-central/rev/2bbb0c0a90df20702df8c8011a8996536a83cb75/devtools/client/debugger/src/utils/editor/source-documents.js#156-161

const doc = editor.createDocument();
setDocument(source.id, doc);
editor.replaceDocument(doc);

setEditorText(editor, source.id, content);
setMode(editor, source, content, symbols);

Creating a document, then setting the text and finally setting the mode seem to trigger some slow path.
Switching to something like

  const doc = editor.createDocument(
    getTextForDocument(source.id, content),
    getModeForDocument(source, content, symbols)
  );
  setDocument(source.id, doc);
  editor.replaceDocument(doc);

makes the operation run as fast as in my test app, with the test still being highlighted


Now for the regression, before, we would hit
https://searchfox.org/mozilla-central/rev/2bbb0c0a90df20702df8c8011a8996536a83cb75/devtools/client/debugger/src/utils/editor/source-documents.js#122-129

// Disable modes for minified files with 1+ million characters Bug 1569829
if (
  content.type === "text" &&
  isMinified(source, content) &&
  content.value.length > 1000000
) {
  return;
}

when trying to set the mode, so that's why the file wasn't highlighted.
We were bailing because isMinified would return true, and now it returns false.

That's because (I guess) of changes in the data structure, while the code in isMinified wasn't updated
https://searchfox.org/mozilla-central/rev/2bbb0c0a90df20702df8c8011a8996536a83cb75/devtools/client/debugger/src/utils/isMinified.js#21
https://searchfox.org/mozilla-central/rev/2bbb0c0a90df20702df8c8011a8996536a83cb75/devtools/client/debugger/src/utils/isMinified.js#18-24

if (
  !sourceTextContent ||
  !isFulfilled(sourceTextContent) ||
  sourceTextContent.value.type !== "text"
) {
  return false;
}

now type is directly in sourceTextContent, and sourceTextContent doesn't seem to hold the state property that is checked in isFulfilled

So I think we should fix isMinified in this bug, and have another bug to be able to highlight bigger file, not setting the text and the mode separately.

Changes in Bug 1754407 caused a performance regression as it now triggered
syntax highlighting of big files, which we avoided before.
This is because we don't call isMinified with the expected type of data, causing
the file to not be seen as minified, and thus highlighting it.

Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Blocks: 1782177

We'll re-enable syntax highlighting for big files in Bug 1782177

Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d28b00fe705b [devtools] Fix call to isMinified in Debugger to prevent performance issue when opening big single-line file. r=jdescottes.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 105 Branch

The patch landed in nightly and beta is affected.
:nchevobbe, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox104 to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(nchevobbe)

(In reply to Release mgmt bot [:suhaib / :marco/ :calixte] from comment #26)

The patch landed in nightly and beta is affected.
:nchevobbe, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox104 to wontfix.

For more information, please visit auto_nag documentation.

sure, let me do that

Flags: needinfo?(nchevobbe)

Comment on attachment 9287600 [details]
Bug 1779173 - [devtools] Fix call to isMinified in Debugger to prevent performance issue when opening big single-line file. r=jdescottes.

Beta/Release Uplift Approval Request

  • User impact if declined: Opening big minified files in the Debugger freezes the browser, even on beefy machine
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: 1. Go to https://addons.mozilla.com/
  1. Open the debugger
  2. Look for a file in addons.mozilla.com > static-frontend that looks like amo-[numbers].js, and open it

Expected results:

The color of the text should stay black, and the browser shouldn't freeze

  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): DevTools bug, the patch only reverts a few changes
    We are working on a performance test (https://phabricator.services.mozilla.com/D153141), that already confirms that the issue is fixed.
  • String changes made/needed:
  • Is Android affected?: Yes
Attachment #9287600 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Comment on attachment 9287600 [details]
Bug 1779173 - [devtools] Fix call to isMinified in Debugger to prevent performance issue when opening big single-line file. r=jdescottes.

Approved for 104.0b5

Attachment #9287600 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

Comment on attachment 9287600 [details]
Bug 1779173 - [devtools] Fix call to isMinified in Debugger to prevent performance issue when opening big single-line file. r=jdescottes.

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: This bug makes it quite hard to debug big files, which are quite frequent nowadays.
  • User impact if declined: Opening big minified files in the Debugger freezes the browser, even on beefy machine, making it quite painful to debug scripts.
  • Fix Landed on Version: 105
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): DevTools bug, the patch only reverts a few changes
Attachment #9287600 - Flags: approval-mozilla-esr102?
Attachment #9287601 - Flags: approval-mozilla-esr102?
Attached image 1779173.gif (deleted) —

Reproduced the issue with Firefox 104.0a1(20220712215641) on Windows 10x64. The browser freezes after performing the steps from comment 28 and comment 0.
The issue is verified fixed with Firefox 105.0a1 (20220801215129) and 104.0b5 (20220801132524) from comment 30 on Windows 10x64, macOS 11 and Ubuntu 21. After following STR from comment 0 and comment 29 the browser no longer freezes and the text is written in black color.

However, switching dev tools tabs multiple times may lead to browser slowness and sometimes low time freezes. Should we file another issue for this and close this one as verified? Attached a screen recording. Thank you!

Flags: needinfo?(nchevobbe)

(In reply to Alexandru Trif, QA [:atrif] from comment #32)

Created attachment 9288069 [details]
1779173.gif

Reproduced the issue with Firefox 104.0a1(20220712215641) on Windows 10x64. The browser freezes after performing the steps from comment 28 and comment 0.
The issue is verified fixed with Firefox 105.0a1 (20220801215129) and 104.0b5 (20220801132524) from comment 30 on Windows 10x64, macOS 11 and Ubuntu 21. After following STR from comment 0 and comment 29 the browser no longer freezes and the text is written in black color.

However, switching dev tools tabs multiple times may lead to browser slowness and sometimes low time freezes. Should we file another issue for this and close this one as verified? Attached a screen recording. Thank you!

yes, it might be different.
it would be great to have a bug with a profile showing the slowness

Flags: needinfo?(nchevobbe)

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #33)

yes, it might be different.
it would be great to have a bug with a profile showing the slowness

Thank you! Filled bug 1782682. Changing flags per comment 32 and comment 33.

Blocks: 1782701

Comment on attachment 9287601 [details]
Bug 1779173 - [devtools] Track opening of big files in custom debugger DAMP test. r=jdescottes.

Revision D153141 was moved to bug 1782701. Setting attachment 9287601 [details] to obsolete.

Attachment #9287601 - Attachment is obsolete: true
Attachment #9287601 - Flags: approval-mozilla-esr102?

Comment on attachment 9287600 [details]
Bug 1779173 - [devtools] Fix call to isMinified in Debugger to prevent performance issue when opening big single-line file. r=jdescottes.

Approved for 102.2esr.

Attachment #9287600 - Flags: approval-mozilla-esr102? → approval-mozilla-esr102+

Verified fixed with 102.2.0esr (20220802204048) from comment 37 on Windows 10x64, macOS 11 and Ubuntu 21. After following STR from comment 0 and comment 29 the browser and debugger no longer freeze and the text is written in black color.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: