1478051 - Rules view is empty when loading https://vilnacollections.yivo.us/test2.html with devtools open

Status: VERIFIED FIXED

(DevTools :: Inspector: Rules, defect, P1)

Description

[Patrick Brosset <:pbro>]

Steps to reproduce:
- Open https://vilnacollections.yivo.us/test2.html
- Open devtools
- Reload the page

--> The rule-view is empty.

Also the browser console shows some JS errors coming from DevTools:

Error while calling actor 'pagestyle's method 'getApplied' content.text is undefined protocol.js:1016:5

fetchStylesheetFromNetworkMonitor@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/stylesheets.js:165:3
fetchStylesheet@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/stylesheets.js:218:16
getSheetText@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/stylesheets.js:140:10
_getText@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/stylesheets.js:507:12
getText@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/stylesheets.js:490:12
getAuthoredCssText@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/styles.js:1281:12
getApplied@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/styles.js:491:13
protocol.js:1019:7

Protocol error (unknownError): content.text is undefined
utils.js:121:3
Protocol error (unknownError): content.text is undefined
rules.js:894:7

Comment 1

[Julian Descottes [:jdescottes]]

Note that simply checking for content.text at https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/devtools/server/actors/stylesheets.js#161 fixes the issue (ie, we are falling back to making an additional call to get the stylesheet)

I think we are getting the response object before its content has finished being received here. I am not sure if the proper fix would be to wait for the response content to arrive, or simply to do an extra request.

Comment 2

[Razvan Caliman [:rcaliman]]

:julian

The solution you suggest (checking for content.text) works well for this particular case. Any reason why not to check for content.size (which is zero in this scenario)?

I'm not sure of the root cause, though I have suspicions on the slow time to first byte (TTFB) of 300ms+ for the stylesheet coupled with the presumed SPDY/H2 on the server that I naively infer from the `X-Friefox-Spdy: h2` Response header.

Potentially related:

The Rule view seems to have issues with slow-to-load CSS files. Artificially inducing a slow TTFB on a stylesheet (say, 1000ms) causes the Rule view to show the default view but it doesn't update once the response comes through. 

I tested with this simple Node server (copy locally into a file called `index.js` and run with `node index.js`):

```
var http = require('http');

const server = http.createServer((req, res) => {
  res.writeHead(200, { 'Content-Type': 'text/css' });
  setTimeout(() => {
    res.end(`html, body{
      background: green;
      width: 100vw;
      width: 100vh;
    }`);
  }, 1000)
}).listen(8080);
```

And tested against this simple file (paste in address bar of new tab after running `node index.js` from above):
data:text/html,<link rel="stylesheet" href="http://localhost:8080" type="text/css"><div>SLOWCSS

For the issue listed in the bug, your solution works. Shall I open a separate bug to track the Rule view failing to react on slow TTFB stylesheets?

Comment 3

[Julian Descottes [:jdescottes]]

(In reply to Razvan Caliman [:rcaliman] from comment #2)
> :julian
> 
> The solution you suggest (checking for content.text) works well for this
> particular case. Any reason why not to check for content.size (which is zero
> in this scenario)?
> 
> I'm not sure of the root cause, though I have suspicions on the slow time to
> first byte (TTFB) of 300ms+ for the stylesheet coupled with the presumed
> SPDY/H2 on the server that I naively infer from the `X-Friefox-Spdy: h2`
> Response header.
> 
> Potentially related:
> 
> The Rule view seems to have issues with slow-to-load CSS files. Artificially
> inducing a slow TTFB on a stylesheet (say, 1000ms) causes the Rule view to
> show the default view but it doesn't update once the response comes through. 
> 
> I tested with this simple Node server (copy locally into a file called
> `index.js` and run with `node index.js`):
> 
> ```
> var http = require('http');
> 
> const server = http.createServer((req, res) => {
>   res.writeHead(200, { 'Content-Type': 'text/css' });
>   setTimeout(() => {
>     res.end(`html, body{
>       background: green;
>       width: 100vw;
>       width: 100vh;
>     }`);
>   }, 1000)
> }).listen(8080);
> ```
> 
> And tested against this simple file (paste in address bar of new tab after
> running `node index.js` from above):
> data:text/html,<link rel="stylesheet" href="http://localhost:8080"
> type="text/css"><div>SLOWCSS
> 
> For the issue listed in the bug, your solution works. Shall I open a
> separate bug to track the Rule view failing to react on slow TTFB
> stylesheets?

Thanks, I agree that's the right thing to do. Checking `content.text` or `content.size` here makes sense. And as long as we are tracking the bigger problem, it's fine with me.

Also I didn't completely track the origin of the regression, but I assume this started with Bug 1306892. And the issue occurs on the Release channel so having a small fix will be great in case we want to uplift it.

Comment 4

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1478051 - Skip using stylesheet from Netmonitor if its content size is zero.

Review commit: https://reviewboard.mozilla.org/r/259374/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259374/

Comment 5

[Julian Descottes [:jdescottes]]

Comment on attachment 8994838 [details]
Bug 1478051 - Skip using stylesheet from Netmonitor if its content size is zero.

https://reviewboard.mozilla.org/r/259374/#review266376

::: devtools/server/actors/stylesheets.js:161
(Diff revision 1)
>    const request = consoleActor.getNetworkEventActorForURL(href);
>    if (!request) {
>      return null;
>    }
>    const content = request._response.content;
> -  if (request._discardResponseBody || request._truncated || !content) {
> +  if (request._discardResponseBody || request._truncated || !content || !content.size) {

Since we are not reading content.size later in this function, I think we should add a comment to explain why this is relevant.

Comment 6

[Razvan Caliman [:rcaliman]]

Comment on attachment 8994838 [details]
Bug 1478051 - Skip using stylesheet from Netmonitor if its content size is zero.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259374/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df5aaa870b52
Skip using stylesheet from Netmonitor if its content size is zero. r=jdescottes

Comment 8

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/df5aaa870b52

Comment 9

[Patrick Brosset <:pbro>]

Thanks for the quick fix Razvan and Julian. We should uplift to beta (62) at least.
Could you request an uplift?

Comment 10

[Razvan Caliman [:rcaliman]]

Comment on attachment 8994838 [details]
Bug 1478051 - Skip using stylesheet from Netmonitor if its content size is zero.

Approval Request Comment
[Feature/Bug causing the regression]:
https://bugzilla.mozilla.org/show_bug.cgi?id=1478051

[User impact if declined]:
Users won't see the CSS updated in the DevTools Rule view on first load for websites which respond very slowly with the CSS files.

[Is this code covered by automated tests?]:
No

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
Yes. See steps in description of Bug 1478051

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
Not risky because there is a fallback in place to re-request the stylesheet which this change intentionally triggers.

[String changes made/needed]:
None

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

STR are in bug 1478051.

Comment 12

[Catalin Sasca, Desktop QA [:csasca]]

I successfully reproduced the issue on Nightly 63.0a1 (2018-07-24) under Windows 10 (x64) using the STR from Comment 0.

The issue is no longer reproducible on latest Nightly 63.0a1 (2018-07-30) under Windows 10 (x64), Ubuntu 16.04 (x64) and macOS 10.10.

Ni myself as a reminder to verify this on Beta as well once the fix is approved.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8994838 [details]
Bug 1478051 - Skip using stylesheet from Netmonitor if its content size is zero.

Fix was verified in Nightly, Beta62+

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2107f137f325

Comment 15

[Catalin Sasca, Desktop QA [:csasca]]

The issue is no longer reproducible on Firefox Beta 62.0b14, under Windows 10 (x64), Ubuntu 16.04 (x64) and macOS 10.12.