Closed Bug 1267058 Opened 4 years ago Closed 4 years ago

"Handler function threw an exception: TypeError: channel.loadInfo is null" errors in Browser Console with enabled prefetch network predictor

Categories

(Core :: Networking, defect, major)

49 Branch
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox47 --- unaffected
firefox48 + disabled
firefox49 + verified

People

(Reporter: Virtual, Assigned: ckerschb)

References

Details

(Keywords: nightly-community, regression, Whiteboard: [necko-active])

Attachments

(1 file)

STR:
1. open Browser Console (Ctrl+Shift+J keyboard shortcut)
2. load any website page, e.g. https://www.mozilla.org/en-US/firefox/nightly/firstrun/
3. reload it a few times
and see these errors in Browser Console:

16:24:24.333 "Handler function threw an exception: TypeError: channel.loadInfo is null
Stack: NetworkMonitor.prototype._createNetworkEvent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-monitor.js:876:1
NetworkMonitor.prototype._onRequestHeader@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-monitor.js:939:5
NetworkMonitor.prototype.observeActivity<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-monitor.js:710:7
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
Line: 876, column: 1"1ThreadSafeDevToolsUtils.js:80

16:24:24.434 "Handler function threw an exception: TypeError: channel.loadInfo is null
Stack: NetworkMonitor.prototype._createNetworkEvent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-monitor.js:876:1
NetworkMonitor.prototype._httpResponseExaminer@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-monitor.js:660:26
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
Line: 876, column: 1"1ThreadSafeDevToolsUtils.js:80


Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a603640aa82d54b224b2325d8a445fd14dd00bc6&tochange=b3b5be1d2027f915c3c54b966fb41de8444923ee


Workaround:
Set "network.predictor.enable-prefetch" to "false" in about:config


[Tracking Requested - why for this release]: Regression
Flags: needinfo?(hurley)
Hrm, it seems that we need to teach the dev tools to ignore network activity related to prefetches.
Flags: needinfo?(hurley)
Assigning to Nick, unless we can find a better owner for this bug.
Assignee: nobody → hurley
Whiteboard: [necko-active]
Nick, I just did a brief search - I suppose we have to extend PredictorLearn() by a contentPolicyType so we can pass the right loadInfo information to NewChannelInternal() here [1]. We probably also want to pass the nsIDocument here [2] so we can pass it as the loadingNode to NewChannelInternal. Actually I suppose we would like to call NS_NewChannel() [3].

Finally, in the end we should call ->AsyncOpen2() on the channel instead of ->AsyncOpen().

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/Predictor.cpp#1335
[2] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/Predictor.cpp#2215
[3] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.h#102
Tracking for 48 and 49 due to the recent regression.

Once you have a patch, please request uplift to aurora if you think it’s safe, so we don’t ship with this regression. Thanks!
Blocks: 1267591
Blocks: 1267754
Christoph - in reading comment 3, and looking at this a bit, I need to clarify some things. Extending PredictorLearn to take a contentPolicyType - is that necessary? PredictorLearn doesn't fire any loads itself, it just saves some data to disk (which, in fairness, could include the contentPolicyType, though that may involve changing the on-disk format). The loads that the predictor fires off (only from the internals of PredictorPredict) are never used directly, they are just stored in the cache. Some other channel, created with an appropriate loadInfo, and contentPolicyType, and, and, and, will consume that data at some point in the future.

If I do need to store the contentPolicyType - is that an OK thing to persist to disk, or is it something that is intended to be in-memory only, and value mappings may change in future gecko versions? (That would get super tricky, super fast).

For the comment about passing the nsIDocument from PredictorLearn - again, we won't (and in this case, can't) have that available when we go to actually create a channel, since that code path is for learning, not predicting. We may, however, be able to derive it from the docshell (which we *do* have when creating a channel) - I haven't looked at that bit yet.
Flags: needinfo?(ckerschb)
Nick, before making any further suggestion I would like to understand what Predictor::Prefetch() actually does? E.g. can this function potentially be triggered by a webpage? If e.g. we are loading an image, is that result than reused in the webpage?

Once I understand what Predictor::Prefetch() does I can definitely provide better advice.
Flags: needinfo?(ckerschb) → needinfo?(hurley)
Yeah, sorry (spent too much time on this code, so I'm obviously assuming you know it as well as I do!)

Quick rundown on the predictor - as the user goes around loading pages, the predictor saves information about those pages on some metadata in the cache entries for the top-level page. So, say I load https://www.mozilla.org/index.html - on the regular cache entry for that URI, the predictor will store a list of resources loaded from that page (images, stylesheets, script) along with things like "when was that page last loaded?" "when was that resource last used by that page?" "have we used that resource all of the last 10 times we loaded that page?". Later on, when the user loads that page again, the predictor takes notice and starts making educated guesses about things we can do to speed up pageload - DNS prefetches, warm up some connections, and actually load some of the resources we're pretty certain we're going to need. It's that last one that Prefetch does. So in a sense, yes, it is triggered by a webpage (based on resources that page has loaded in the past), but it is *not* triggered directly by a webpage in the sense that there is no markup or script that can cause Predictor::Prefetch to be called (this is *not* hooked up to <link rel=prefetch>, for example). However, the stuff that is put in the cache by Predictor::Prefetch will (hopefully) be used in a webpage later, but not directly from the channel Predictor::Prefetch opens, only indirectly through the cache entry created by that channel.

Prefetch does the following: create a channel, AsyncOpen it, and wait for it to finish. Once OnStopRequest fires, if the HTTP status of the load was 200, we call ForceCacheEntryValidFor(seconds) on the nsICachingChannel. The default value for "seconds" is 10 (preffable).

Later (hopefully) another channel will be opened for the same resource, from the script loader, or stylesheet loader, or, or... When that channel goes and sees it has a cache entry, as long as the entry is still "forced valid" (the number of seconds above hasn't elapsed), it will use that cache entry *without* sending a conditional request (if it would have otherwise sent one).

Does this help with understanding?

(Let the dueling ni?s begin!)
Flags: needinfo?(hurley) → needinfo?(ckerschb)
First off, I think contentPolicyType is the least of the worries here. If we're going to add security information, then adding loadingPrincipal and triggeringPrincipal is much more important.

That said, if the resource is just saved to disk, and never used without first doing the normal security checks that we always do before starting a necko request, then I don't think that we need to worry about adding any "correct" security information.

We can simply use TYPE_OTHER and use the system principal as the loadingPrincipal+triggeringPrincipal.


The only thing that I'd worry about is that these network requests might be a privacy issue. The user might have some addon which usually blocks requests using nsIContentPolicy. These checks won't be done here, especially with the correct security information, and so the requests will not get blocked. These requests might leak information that the user didn't want shared, especially when the request happens over unencrypted http.

But that's not a new problem. It exists just as much already. And I'd imagine it's something that you guys have debated and acted upon as needed. So I think we should treat that as a separate issue.

The other thing that I'd worry about, though again, not yet, is userContextId. We need to make sure we use the correct cookie jar when making these requests. But again, that seems like a separate problem that we should solve in a bug specifically for that.


In short, just use the system principal, TYPE_OTHER and ALLOW_CROSS_ORIGIN_DATA_IS_NULL here.
(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #7)
> Does this help with understanding?

Yes, that helps indeed. I had a different concept in my head of what the predictor actually does. Anyway, in that case we can use the SystemPrincipal as the LoadingPrincipal/TriggeringPrincipal and TYPE_OTHER as the contentPolicyType.
Flags: needinfo?(ckerschb)
Attachment #8748636 - Flags: review?(hurley)
Comment on attachment 8748636 [details] [diff] [review]
bug_1267058_asyncopen2_predictor_prefetch.patch

Review of attachment 8748636 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome, thanks for the info, and extra thanks for writing the patch!
Attachment #8748636 - Flags: review?(hurley) → review+
Disabled on aurora thanks to bug 1268208
https://hg.mozilla.org/mozilla-central/rev/01eabdfce479
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee: hurley → ckerschb
Status: RESOLVED → VERIFIED
Has Regression Range: --- → yes
Has STR: --- → yes
Version: 48 Branch → 49 Branch
You need to log in before you can comment on or make changes to this bug.