Closed
Bug 1431758
Opened 6 years ago
Closed 6 years ago
Loading https://fr.moleskine.com/home with the inspector open breaks the site and tab
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(firefox-esr52 unaffected, firefox58 unaffected, firefox59 fixed, firefox60 fixed)
RESOLVED
FIXED
Firefox 60
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox58 | --- | unaffected |
firefox59 | --- | fixed |
firefox60 | --- | fixed |
People
(Reporter: pbro, Assigned: jdescottes)
References
Details
(Keywords: regression)
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
jryans
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
This one is pretty serious. I wonder what this site does that breaks the inspector and tab but it does. Follow these steps: - open https://fr.moleskine.com/home --> loads fine - open the inspector - reload the page --> problem here! The page somewhat loads but appears blank, the inspector is empty too, and the tab is broken. Hitting the reload button does do anything. This only happens when the inspector is open. I tried with the console and it worked fine. It also does it if you first go to any site, then open the inspector, then navigate to that site. So it's not just a reload thing, but a load thing.
Assignee | ||
Comment 1•6 years ago
|
||
Seems to be FF59 only.
status-firefox58:
--- → unaffected
status-firefox59:
--- → affected
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•6 years ago
|
||
Still investigating, this bug is linked to 1429254, but I don't think the code of the patch or of the blocking bug is the issue here. Still investigating. Can easily be reproduced in isolation with an empty page loading the following stylesheet: data:text/html,<link rel="stylesheet" type="text/css" href="https://fr.moleskine.com/file/v3647388651203621628/css/storefront.css">
Assignee | ||
Comment 3•6 years ago
|
||
In my reduced test case, I have a huge CSS file with 130.000 lines filled with: #fake { font-size: 0.93em; position: absolute; top: 17px; right: 5px; color: #3b3b3b; } And after that: body { background: wheat; } The key to reproduce the bug is to have a huge file, and make sure that one of the rules needed by the ruleview is at the end of the file. There will be two calls to getTextAtLineColumn in devtools/server/actors/styles.js (https://searchfox.org/mozilla-central/rev/2031c0f517185b2bd0e8f6f92f9491b3410c1f7f/devtools/server/actors/styles.js#1692-1702). The second one will made with a truncated version of the CSS text, which is missing the line that contains the content we need (here the body rule). The regexp exec() done in the function will never finish and will block the content process. Still not sure why the cssText gets truncated when we reach getTextAtLineColumn for a second time.
Assignee | ||
Updated•6 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
See Also: → 1430776
Assignee | ||
Updated•6 years ago
|
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Assignee | ||
Comment 5•6 years ago
|
||
So the issue is that the second call uses a stylesheet text retrieved from the netmonitor, and the netmonitor never gets the full content of the request. This seems to depend on the size of the content, the location as well (different in localhost or in a remote server etc...). I set up my example page at: http://juliandescottes.github.io/devtools-demos/css-bug/ You can open the netmonitor and inspect the response tab for styles.css. The line count *should* be 133,930. Right now, the netmonitor only has content until line 84,000 (changes every time I reload). Since we are now relying on the netmonitor to fetch stylesheet text (Bug 1306892), the inspector and style editor only get a truncated stylesheet. I'll try to identify why the netmonitor only records partial content of the request.
See Also: 1430776 →
Assignee | ||
Comment 6•6 years ago
|
||
As pointed out by jryans, the limit is set on purpose at https://searchfox.org/mozilla-central/source/devtools/shared/webconsole/network-monitor.js#39. We need to detect this kind requests and avoid relying on the netmonitor if the response body is not complete.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8944110 -
Flags: review?(jryans)
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8944110 [details] Bug 1431758 - do not use netmonitor data to fetch stylesheets over 1MB; https://reviewboard.mozilla.org/r/214436/#review220254 Thanks for working on this! :) It's great to have a test for this case as well. Let's ensure a fix for this gets into 59. We might need an uplift depending on the merge time.
Attachment #8944110 -
Flags: review?(jryans) → review+
Assignee | ||
Comment 9•6 years ago
|
||
Thanks for the review! I'll check back and get this gets uplifted to 59 if needed.
Comment 10•6 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/41262016e149 do not use netmonitor data to fetch stylesheets over 1MB;r=jryans
Comment 11•6 years ago
|
||
Backed out for build bustage https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1ec6077296c95cc68489bf05b4f64850af03f9b0 Log: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1ec6077296c95cc68489bf05b4f64850af03f9b0 Rev: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=41262016e149a228b6f63fea39f3a09be800e787
Flags: needinfo?(jdescottes)
Assignee | ||
Comment 12•6 years ago
|
||
Ah sorry forgot to add the new file :(
Flags: needinfo?(jdescottes)
Comment hidden (mozreview-request) |
Comment 14•6 years ago
|
||
This also has a test failure: https://treeherder.mozilla.org/logviewer.html#?job_id=157750187&repo=autoland
Flags: needinfo?(jdescottes)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•6 years ago
|
||
The test doesn't make sense without the missing file. I'm not sure why/how it managed to go past the build step despite the missing test file. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e313daaca8a9fa04fcb6a380db2982ab10534b7
Flags: needinfo?(jdescottes)
Comment 17•6 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/55eb10f2b439 do not use netmonitor data to fetch stylesheets over 1MB;r=jryans
Comment 18•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/55eb10f2b439
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Assignee | ||
Comment 19•6 years ago
|
||
Comment on attachment 8944110 [details] Bug 1431758 - do not use netmonitor data to fetch stylesheets over 1MB; Impact and STRs changed a bit after Bug 1430776 got resolved. The tab no longer freezes, instead, rules are simply missing from the ruleview. I still think this is worth uplifting. New STRs: - go to http://juliandescottes.github.io/devtools-demos/css-bug/ - open StyleEditor - reload the page - select styles.css, scroll to the bottom ER: the body {} selector is visible and last line is 133,930 AR: the body {} selector is not visible and last line is lower than 133,930 Approval Request Comment [Feature/Bug causing the regression]: Bug 1429254 [User impact if declined]: DevTools will be missing rules for stylesheets > 1MB [Is this code covered by automated tests?]: yes, added in the same patch [Has the fix been verified in Nightly?]: not yet [Needs manual test from QE? If yes, steps to reproduce]: no. automated test coverage is enough, STRs mentionned if needed. [List of other uplifts needed for the feature/fix]:none [Is the change risky?]:no [Why is the change risky/not risky?]:we are reusing the existing mechanism to fetch stylesheets if a stylesheet is over 1MB. Stylesheets were always fetched in this manner before FF59. [String changes made/needed]:none
Attachment #8944110 -
Flags: approval-mozilla-beta?
Comment 20•6 years ago
|
||
Comment on attachment 8944110 [details] Bug 1431758 - do not use netmonitor data to fetch stylesheets over 1MB; Fix for a recent regression, has test coverage, let's uplift this for the beta 4 build.
Attachment #8944110 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•6 years ago
|
Keywords: regression
Updated•6 years ago
|
status-firefox-esr52:
--- → unaffected
Comment 21•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/f060ec470623
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•