Closed Bug 1431758 Opened 2 years ago Closed 2 years ago

Loading https://fr.moleskine.com/home with the inspector open breaks the site and tab

Categories

(DevTools :: Inspector, defect)

defect
Not set

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)

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.
Seems to be FF59 only.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Blocks: 1429254
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">
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.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1430776
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
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.
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.
Attachment #8944110 - Flags: review?(jryans)
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+
Thanks for the review! I'll check back and get this gets uplifted to 59 if needed.
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
Ah sorry forgot to add the new file :(
Flags: needinfo?(jdescottes)
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)
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
https://hg.mozilla.org/mozilla-central/rev/55eb10f2b439
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
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 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+
Keywords: regression
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.