Closed Bug 1498565 Opened 6 years ago Closed 6 years ago

Showing XML response payload freezes Firefox

Categories

(DevTools :: Netmonitor, defect, P2)

Product:
DevTools
Component:
Netmonitor
Version:
64 Branch
Type:
defect

Tracking

(firefox-esr60 unaffected, firefox62 unaffected, firefox63 unaffected, firefox64 wontfix, firefox65 wontfix, firefox66 wontfix, firefox67 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 67
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- unaffected
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- fixed

People

(Reporter: nachtigall, Assigned: amychan331)

References

Details

(Keywords: regression)

Attachments

(6 files, 4 obsolete files)

Firefox freezes
(deleted), image/png
Details
Response payload error with large XML data
(deleted), image/png
Details
Getting Firefox to stop freezing when large file is loaded in Response tab
(deleted), text/x-phabricator-request
Details
Double scrollbar
(deleted), image/png
Details
overlap text container.png
(deleted), image/png
Details
response-freeze.patch
(deleted), patch
Details | Diff | Splinter Review
Attached image Firefox freezes (deleted) —
I ran mozregression and it points me to https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=cfe90e5953d9c3705a52f2db2a82888ad40a8898&tochange=255ba5eac28341ed24c6a88327cb18b0fc9c124b meaning that this bug is a regression of bug 1479421 STR: Open a large XML response in the DevTools' network response tab. For instance 1. Open Network in Devtools 2. Filter on XHR 3. Open https://www.umweltpaktapp.bayern.de/ 4. Click to the request with `appservice?` in it 5. Click on the "Response" tab AR: Browser hangs and freezing spinning wheel is shown. See screenshot. ER: Response should load instantly like it did or does in Non-Nightly Firefox.
Blocks: 1479421
status-firefox62: --- → unaffected
status-firefox63: --- → unaffected
status-firefox64: --- → affected
Version: 63 Branch → 64 Branch
Amy, can you please look a this? It's related to the work you did in bug 1479421 Thanks, Honza
Flags: needinfo?(amy_yyc)
Priority: -- → P2
Following the steps, I can also replicate the issue. The response tab do output the XML with given time, but the error alert stills popped up. I will attach the error code from the crash as image (for some reason, when I went back later, the same website don't seem to be getting the same large XML data? But I have a snapshot from earlier for record.) After some investigation with the error code and my CSS changes before, I think it may be from the SourceEditor.js or CodeMirror. My previous bug changes turns editor-row-container class visible. In PropertiesView.js, the method that renders .editor-row-container also renders SourceEditor. In the error code attached, SourceEditor line 41 leads to this.editorTimeout(), which exists for "Delay to CodeMirror initialization content to prevent UI freezing". I will try to continue investigating from this angle.
Been trying to go through the files showed in the error code and related files, like the updateHeightsInViewport() method in codemirror (as indicated by the error code), Responsive Panel, Properties Views, and Treeview. At first I thought it may just be that the file may just be too big, but JSON didn't have the same issue. In addition, while it takes some time, the XML code do eventually load and appear in the Response Panel. I am currently lost about what direction I should go to resolve this.
Flags: needinfo?(amy_yyc) → needinfo?(odvarko)
Sorry for the delay! (In reply to Amy Chan (:amychan331) from comment #4) > Been trying to go through the files showed in the error code and related > files, like the updateHeightsInViewport() method in codemirror (as indicated > by the error code), Responsive Panel, Properties Views, and Treeview. At > first I thought it may just be that the file may just be too big, but JSON > didn't have the same issue. I think that it actually is problem with a big file. I tried JSON too and if it's all at one line only (not pretty printed, no \n lines), it works well. As soon as the Response panel (ie CodeMirror) needs to display pretty printed JSON (many lines) it freezes the same way as XML. It's CodeMirror fault. I don't think we can fix CodeMirror and so we should avoid using it for big files (and/or files with many lines) The place where the SourceEditor (wrapper for CodeMirror) is here: https://searchfox.org/mozilla-central/rev/d850d799a0009f851b5535580e0a8b4bb2c591d7/devtools/client/netmonitor/src/components/PropertiesView.js#120 I am thinking about something like as follows: let responseTextComponent = SourceEditor(value); if (value.text.length > 1024 * 10) { responseTextComponent = pre({}, value.text); } // Display source editor when specifying to EDITOR_CONFIG_ID along with config if (level === 1 && name === EDITOR_CONFIG_ID) { return ( tr({ key: EDITOR_CONFIG_ID, className: "editor-row-container" }, td({ colSpan: 2 }, responseTextComponent ) ) ); } - use pre() element instead of the SourceEditor - use it when the response.text is bigger than 10KB - we should come up with a value that is safe enough - not sure whether we should count lines, we should experiment Honza
Flags: needinfo?(odvarko) → needinfo?(amy_yyc)
When large files's Response tab in Network Devtools is open, Firefox freezes. This checks for file size & switches from SourceEditor to pre() when the files is too big.
The same also happened when I was working with it earlier - I tried to the change the value in this.editor of SourceEditor so it only grabs part of the text (I did 1024 * 100). The Response tab was a bit slow, but the XML file loaded without error. However, I didn't know how to improve the performance anymore than it does - I tried the pre() as suggested and it did an amazing job in getting the file to load without problem. I tried it with both the XML site above and a 1MG JSON file.
Flags: needinfo?(amy_yyc)
Assignee: nobody → amy_yyc
@Amy, this is very close to landing. Please see my inline comments in Phabricator. Thanks! Honza
Flags: needinfo?(amy_yyc)
Sorry about the delayed response. Got the latest update up. Regarding the comment about "There is no horizontal scrollbar for the <pre> element", the horizontal scrollbar do show on my side with and without pre, but I have to scroll down to the bottom to see it. Do you want the scrollbar to appears without scrolling to the bottom?
Flags: needinfo?(amy_yyc) → needinfo?(odvarko)
(In reply to Amy Chan (:amychan331) from comment #9) > Sorry about the delayed response. Got the latest update up. Regarding the > comment about "There is no horizontal scrollbar for the <pre> element", the > horizontal scrollbar do show on my side with and without pre, but I have to > scroll down to the bottom to see it. Do you want the scrollbar to appears > without scrolling to the bottom? Yes, see my response in Phabricator. Honza
Flags: needinfo?(odvarko)
Finally go the horizontal scrollbar to appear without scrolling! Took a while to figure out which element to change height and why the height always seems just a bit longer - it seems the rows of toolbars and tabs above it added to the overall height of the source editor container, so I have to subtract those row elements for the height.
Flags: needinfo?(odvarko)
Attached image Double scrollbar (deleted) —
Flags: needinfo?(odvarko)
@Florens, any tip how to fix this double scrollbar problem? (see the attached screenshot, in previous comment) Here is the patch introducing it: https://phabricator.services.mozilla.com/D12450 This line is problematic and we should avoid using hardcoded value: height: calc( 100vh - var(--primary-toolbar-height) - 44px ); Honza
Flags: needinfo?(florens)
So the Response tab content is a container that can contain between 1 and 3 elements: 1. The error message. If present, its height will depend on the font-size, the actual font and platform, whether text wraps to two lines (so that depends on the sidebar width), etc. So we shouldn't hardcode that, as you wrote. 2. The "Preview" pane, if present. 3. The "Response payload" pane, if present. I don't have time to check what the current DOM structure and CSS is like right now, but conceptually this can be coded as: <container> <message>Woops, something happened</message> <preview> <header>Preview</header> <content>...</content> </preview> <payload> <header>Payload</header> <content>...</content> </payload> </container> Where any of the first two branches may or may not exist. We should be able to lay out these containers dynamically with Flexbox. If we use display:contents on the `preview` and `payload` containers, we end up with up to 5 elements, 3 using `flex: 0 0 auto` (message, headers) and 2 using e.g. `flex: 1 1 40%` (content). See a demo here: https://codepen.io/anon/pen/bOEpVZ Two important bits: - We need a way to set the container to 100% height. Maybe we can use `height:100%` in this context, maybe the container is itself a flex child that can use flex-grow:1, or maybe we'll need something more tricky. - We need the same non-auto flex-basis value on all "content" elements, or their height my end up depending on their content instead. Hope this helps.
Flags: needinfo?(florens)
I looked into the dom structure of the panel, and determined which element of it corresponds to the element in the Codepen example by Florens. That would be: container => .network-monitor .panel-container preview => .network-monitor .tree-container || .network-monitor .tree-container .treeTable || .network-monitor .tree-container .treeTable tbody div => .network-monitor .tree-container .treeTable .editor-row-container And I have tried different variation of css by changing the display, flex, and height values in different classes from the one mentioned above. The panel either don't have a scrollbar at all, or have a separate double scrollbar that run the full height of the preview panel like the way it was initially. Besides the failed attempts, there are some additional concerns I discovered on the way: 1) The DOM structure is: ---------------------------------------------------- <div class="panel-container"> ... <tbody> <tr>JSON</tr> <td>property: value</td> <td>property: value</td> <td>property: value</td> ... <tr>Response Payload</tr> <tr class="editor-row-container">contents</tr> </tbody> ... </div> ---------------------------------------------------- Notice that the Response Payload panel have a parent, tr.editor-row-container. JSON panel is composed of just a list of td. If I want a 50% height from both panel, I can try for 50% height in tr.editor-row-container, but I can't do that with the changing rows of td since there isn't a parent element that wraps it. Should I add one? If I add one, I am worry about it affecting other panels, which resulted in my next discovery... 2) The Header and Response tab both uses .panel-container, tbody, tr, and td. They share the same scroll behavior pattern, but the Headers have an overview element above. Visually, it wouldn't work well trying to force its 2 panels and overview to be all visible the way that the Codepen example shows. Should I aim to make the change specify to Response tab? 3) Looking into the last concern made me wonder if my current changes affect Header already, so I tried to run this without my initial patch (D12450), with cause the performance regression. The behavior of having one vertical scrollbar that require scrolling down to see a second panel or horizontal scrollbar existed before this in Header and Response tab. I am having problems trying to get the scrollbar, so I am not sure how long I will take to make it happen. Should we open a separate bug so this is not holding up a regression bug fix that is causing Firefox to freeze?
Flags: needinfo?(odvarko)

@Florens: can you please look at comment #15, thanks!

Honza

Flags: needinfo?(odvarko) → needinfo?(florens)

Too late for Fx65 at this point with the RC coming up next week :(

status-firefox65: affected → wontfix

@Amy: I tested your patch and found a way how to fix the scroll bar problem.

Can you please undo all your changes and keep just the following:

+
+.network-monitor .tree-container .treeTable .editor-row-container pre {
+  padding-left: 5px;
+  position: absolute;
+}
+
  • Note the position: absolute;

Thanks!
Honza

Flags: needinfo?(florens) → needinfo?(amy_yyc)
Attached image overlap text container.png (deleted) —

Unfortunately, after undoing the changes and applying the positioning, the text in the container all overlaps each other into a single line - I have attach the image of what happened. I have also tried to move the absolute positioning to ".network-monitor .tree-container .treeTable .editor-row-container" and ".network-monitor .tree-container .treeTable tr:last-child.editor-row-container". The text either overlap like the attached image, or it overlaps the text in the first row container panel. I have also tried to change the height to 100% while absolute positioning with similar results.

Flags: needinfo?(amy_yyc)
Attached patch response-freeze.patch (obsolete) (deleted) — — Splinter Review

(In reply to Amy Chan (:amychan331) from comment #20)

Unfortunately, after undoing the changes and applying the positioning, the text in the container all overlaps each other into a single line

I can't reproduce this.

Can you try my patch and see if it also causes the issue?
If yes, can you point me to a test case I could use to repro this on my machine?

Thanks!
Honza

Flags: needinfo?(amy_yyc)

I have tried the patch. I have also update to the newest Nightly, had the laptop update and mach build it overnight, then apply the patch again. Then I also add in the position: absolute. The same result as when I first started - double scrollbar where the horizontal scrollbar is viewable only when scroll to the bottom.
What do you mean by test case?

Flags: needinfo?(amy_yyc) → needinfo?(odvarko)

(In reply to Amy Chan (:amychan331) from comment #22)

What do you mean by test case?
Specific page I could use for testing (+ some instructions about what exactly to do to see the problem)

Thanks
Honza

Flags: needinfo?(odvarko) → needinfo?(amy_yyc)

I have been using the MOF doc for omg.org at https://www.omg.org/spec/CWM/20020501/02-05-01.xml for large XML file, and either use the https://www.umweltpaktapp.bayern.de/ link as Jens did in the initial comment or pick a file in https://github.com/zemirco/sf-city-lots-json for large JSON files.

Flags: needinfo?(amy_yyc)
Attached patch response-freeze2.patch (obsolete) (deleted) — — Splinter Review

Amy, I am attaching a new patch, can you please try it out?

Here are my STRs

  1. Load http://janodvarko.cz/tests/bugzilla/1514750/
  2. Open the Toolbox and select the Network panel
  3. Click the button on the page
  4. Select the request and the Response side panel
  5. The UI should render the (big) JSON response as plain text.

Honza

Flags: needinfo?(amy_yyc)
Attached patch response-freeze.patch (obsolete) (deleted) — — Splinter Review

New patch attached.

Amy, can you please test this patch?
Thanks!

Honza

I applied the patch and tested with both XML and JSON file. The result for both XML and JSON is similar to before - vertical scrollbar visible, horizontal visible only if scroll down. I use http://api.nobelprize.org/v1/prize.json for JSON, https://www.omg.org/spec/CWM/20020501/02-05-01.xml for XML.

Flags: needinfo?(amy_yyc)

Hey amy, any chance we will get this fixed for 66, or should I mark it fix-optional?

Flags: needinfo?(amy_yyc)

At this rate, I feel that Honza would probably know better I do. The response payload panel now no longer freeze with large XML files, but I haven't been able to make progress in term of making the horizontal bar visible without scrolling down. I have tried various flexbox and css implementation - even trying out something different everyday for a week at one point in fact - but I am completely stuck! So I am open to someone else working on this or offer suggestions.

Flags: needinfo?(amy_yyc)
Attached patch response-freeze.patch (obsolete) (deleted) — — Splinter Review

Patch coming...

Flags: needinfo?(amy_yyc)
Attached patch response-freeze.patch (deleted) — — Splinter Review

(In reply to Amy Chan (:amychan331) from comment #30)

The response payload panel now no longer freeze with large XML files, but I haven't been able to make progress in term of making the horizontal bar visible without scrolling down.

Understand. I think that it's more important at this point to fix the "freezing" problem and create a follow up for the scrollbar issue.

Can you please look at the attached patch (it's yours but updated), test it, comment it and if you don't see any issues, upload it on Phabricator, please?

Try push looks good to me:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=afaa4ac22dbaa13bb3c2a95d4553aa4a2c5014c2

Thanks for the help!
Honza

Sorry for the late response. I tested the patch. It worked fine and I uploaded to Phabricator.

Flags: needinfo?(amy_yyc)
Pushed by jodvarko@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/980be6fb9210 Getting Firefox to stop freezing when large file is loaded in Response tab r=Honza

https://hg.mozilla.org/mozilla-central/rev/980be6fb9210

Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox67: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67

A follow up bug for the scrollbar issue is now open at https://bugzilla.mozilla.org/show_bug.cgi?id=1531526 if anyone wants to work on it!

Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: