Closed Bug 1372115 Opened 3 years ago Closed 3 years ago

Error in console NS_ERROR_INVALID_CONTENT_ENCODING [nsIStreamListener.onDataAvailable] visiting facebook w/ bytecode cache

Categories

(DevTools :: Netmonitor, defect, P1)

55 Branch
x86
Windows 10
defect

Tracking

(firefox57 disabled, firefox58 verified, firefox59 verified)

VERIFIED FIXED
Firefox 59
Tracking Status
firefox57 --- disabled
firefox58 --- verified
firefox59 --- verified

People

(Reporter: ananuti, Assigned: ochameau)

References

Details

Attachments

(2 files)

STR:
1. set 
   - dom.script_loader.bytecode_cache.enabled = true &&
   - dom.script_loader.bytecode_cache.strategy = -1
2. visited https://www.facebook.com/.

I get this in my console:

NS_ERROR_INVALID_CONTENT_ENCODING: Component returned failure code: 0x804b001b (NS_ERROR_INVALID_CONTENT_ENCODING) [nsIStreamListener.onDataAvailable]
network-monitor.js:658

```
onInputStreamReady: function (stream) {
    if (!(stream instanceof Ci.nsIAsyncInputStream) || !this.httpActivity) {
      return;
    }

    let available = -1;
    try {
      // This may throw if the stream is closed normally or due to an error.
      available = stream.available();
    } catch (ex) {
      // Ignore.
    }

    if (available != -1) {
      if (available != 0) {
        if (this.converter) {
          this.converter.onDataAvailable(this.request, null, stream,
                                         this.offset, available);
        } else {
          this.onDataAvailable(this.request, null, stream, this.offset,
                               available);
        }
      }
      this.offset += available;
      this.setAsyncListener(stream, this);
    } else {
      this.onStreamClose();
      this.offset = 0;
    }
  },
```

Line 658 is the this.converter.onDataAvailable(this.request, null, stream, line.
I can reproduce it, but the STR should include:
 1.5 Open the devtools (console panel)

It sounds that the the "network-monitor.js" file is meant for monitoring the content which is being request from the network.
Looking at the nsHttp log, this seems to be coming from the BrotliHandler:

[Main Thread]: D/nsHttp nsHttpCompressConv 0x3cd64d0 OnDataAvailable 5550
[Main Thread]: D/nsHttp nsHttpCompresssConv 0x3cd64d0 brotlihandler decompress 5550
[Main Thread]: D/nsHttp nsHttpCompresssConv 0x3cd64d0 brotlihandler decompress rv=0 out=0
[Main Thread]: D/nsHttp nsHttpCompressConv 0x3cd64d0 marking invalid encoding (¹)
JavaScript error: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-monitor.js, line 658: NS_ERROR_INVALID_CONTENT_ENCODING: Component returned failure code: 0x804b001b (NS_ERROR_INVALID_CONTENT_ENCODING) [nsIStreamListener.onDataAvailable]

¹ http://searchfox.org/mozilla-central/source/netwerk/streamconv/converters/nsHTTPCompressConv.cpp#217
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(mcmanus)
not sure what info you need, but the error just means you're trying to pass something through the brotli decoder that it doesn't think is valid brotli
Flags: needinfo?(mcmanus)
(In reply to Patrick McManus [:mcmanus] from comment #2)
> not sure what info you need, but the error just means you're trying to pass
> something through the brotli decoder that it doesn't think is valid brotli

I added you because you wrote this code in Bug 366559, such that you can help with the interpretation of this message.

I guess the problem here might be that we are loading alternate data from necko, instead of the brotli compressed source, and we might still attempt to decompress the content generated by the JS engine as a brotli encoded stream.

So I presume we should probably check for the alternative data type[1] to skip the brotli decoder.

[1] http://searchfox.org/mozilla-central/source/netwerk/base/nsICacheInfoChannel.idl#91
Assignee: nobody → valentin.gosu
Flags: needinfo?(valentin.gosu)
Whiteboard: [necko-active]
Component: Networking: HTTP → Developer Tools: Netmonitor
Product: Core → Firefox
Comment on attachment 8879490 [details] [diff] [review]
bug1372115-wip.patch

This patch seems to make console error go away, but I'm not entirely sure it's correct.
Hi Honza, can you get someone to take this bug and/or review this patch? Thanks!
Assignee: valentin.gosu → nobody
Flags: needinfo?(odvarko)
Whiteboard: [necko-active]
@Michal: can you please take a quick look at this change?

Honza
Flags: needinfo?(odvarko) → needinfo?(michal.novotny)
I guess Valentin meant somebody familiar with the network monitor code. Anyway, if the channel can be also something else than nsHttpChannel and it doesn't implement nsICacheInfoChannel the condition should be maybe something like this:

     if (!this.httpActivity.fromServiceWorker &&
         channel instanceof Ci.nsIEncodedChannel &&
         channel.contentEncodings &&
         !channel.applyConversion &&
         (!(channel instanceof Ci.nsICacheInfoChannel) ||
         !channel.alternativeDataType)) {
Flags: needinfo?(michal.novotny)
Assignee: nobody → poirot.alex
All bytecode cached JS files in netmonitor have their response content broken (Response panel is empty).
It also breaks some other features like bug 1417409.

Applying comment 8's patch removes the exception, fix bug 1417409, but the content displayed in Response panel is wrong.
I imagine we see the bytecode cache content instead of original JS file.

:nbp, any idea if we can still access the original JS file content through the `channel` object?
Flags: needinfo?(nicolas.b.pierron)
(In reply to Alexandre Poirot [:ochameau] from comment #9)
> All bytecode cached JS files in netmonitor have their response content
> broken (Response panel is empty).
> It also breaks some other features like bug 1417409.
> 
> Applying comment 8's patch removes the exception, fix bug 1417409, but the
> content displayed in Response panel is wrong.
> I imagine we see the bytecode cache content instead of original JS file.
> 
> :nbp, any idea if we can still access the original JS file content through
> the `channel` object?

In general we cannot, because the alternate data is being streamed instead of the source.
In practice we could hack something for the JS alternate data because it does contain the source, but this would be an awful dirty hack that I would recommend against.

In such case, the best would be to start a new request for the same resource without setting any preferred alternate data type, or to intercept the channel creation and remove the preferred alternate data type.

Valentin, Honza or Michal would be the best persons to ask about this.
Valentin, what would be the next step here?
Flags: needinfo?(nicolas.b.pierron) → needinfo?(valentin.gosu)
Comment on attachment 8928512 [details]
Bug 1372115 - Prevent exception in network monitor when JS file is loaded throught the bytecode cache.

In this new patch I use NetworkHelper.readFromCache, defined here:
https://searchfox.org/mozilla-central/source/devtools/shared/webconsole/network-helper.js#297
To force loading request from cache when it is a bytecodecache request.

Not sure it is the best way to do it, but with that, we can see the real JS content.
Blocks: 1411889
Comment on attachment 8928512 [details]
Bug 1372115 - Prevent exception in network monitor when JS file is loaded throught the bytecode cache.

Not sure it is a perfect solution, but still much better than the current state where many things are broken...
We can always revisit than once we get more feedback.
Attachment #8928512 - Flags: review?(odvarko)
Netmonitor get broken in multiple ways when there is one request for a bytecode cached file (like bug 1417409, or bytecode cached request having multiple columns empty).
Comment 0 mentions preferences to toggle, but since then, bytecode cache has been turned on by default.
Priority: P3 → P1
Comment on attachment 8928512 [details]
Bug 1372115 - Prevent exception in network monitor when JS file is loaded throught the bytecode cache.

https://reviewboard.mozilla.org/r/199768/#review206352

Fixes the problem for me, thanks Alex.

R+ assuming try is green

Honza
Attachment #8928512 - Flags: review?(odvarko) → review+
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ec038106d24c
Prevent exception in network monitor when JS file is loaded throught the bytecode cache. r=Honza
https://hg.mozilla.org/mozilla-central/rev/ec038106d24c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Mass won't fix for 58. Let it ride the train.
Flags: needinfo?(valentin.gosu)
Status: RESOLVED → VERIFIED
Comment on attachment 8928512 [details]
Bug 1372115 - Prevent exception in network monitor when JS file is loaded throught the bytecode cache.

Approval Request Comment
[Feature/Bug causing the regression]:
  This bug appeared with the creation of the bytecode cache, but become really visible once we enabled it by default in bug 1405738.
[User impact if declined]:
  Broken netmonitor, in multiple ways like comment 9 (missing response content) or bug 1426069 (broken statistics panel)
[Is this code covered by automated tests?]:
  Not this particular branch, specific to js files stored in bytecode.
[Has the fix been verified in Nightly?]:
  Yes.
[Needs manual test from QE? If yes, steps to reproduce]: 
  See bug 1426069 comment 0.
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
  Not much.
[Why is the change risky/not risky?]:
  Limited to netmonitor. Written in a way to act only against bytecoded resources.
[String changes made/needed]:
  No.
Attachment #8928512 - Flags: approval-mozilla-beta?
Comment on attachment 8928512 [details]
Bug 1372115 - Prevent exception in network monitor when JS file is loaded throught the bytecode cache.

fix netmonitor vs jsbc, beta58+
Attachment #8928512 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I reproduced this issue on beta 58.0b11 on Windows 10 x64, according to the comment 22. I retested on beta 58.0b13 and this issue is fixed.
Flags: qe-verify+
Blocks: 1429365
Duplicate of this bug: 1426069
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.