Closed Bug 1266196 Opened 8 years ago Closed 8 years ago

about:cache channels cannot be instantiated in parallel, make it multi-instance capable

Categories

(Core :: Networking: Cache, defect, P1)

48 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla49
Iteration:
49.2 - May 23
Tracking Status
firefox48 + verified
firefox49 + verified

People

(Reporter: alice0775, Assigned: mayhemer)

References

Details

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

Attachments

(2 files, 3 obsolete files)

[Tracking Requested - why for this release]: 


Steps To Reproduce:
1. Open about:home and any other about: page
2. Open about:cache in the tab

Actual Results:
No cache information are reported. But Title and form is displayed.
Reloading page displays the cache information.

Expected Results:
Cache information should be reported

Regression window:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=e97898890d476686262db5d9a870d35fdc4da8b9&tochange=f8ec60f2667eb6c1195e24a08e866caea6cb034f

Suspect: Bug 1262009
Flags: needinfo?(tanvi)
Flags: needinfo?(bgrinstead)
Flags: needinfo?(MattN+bmo)
Is there an error in the Browser Console?
Flags: needinfo?(MattN+bmo)
(In reply to Matthew N. [:MattN] (behind on reviews) from comment #2)
> Is there an error in the Browser Console?

Warning: NetUtil.asyncFetch() requires the channel to have one of the security flags set in the loadinfo (see nsILoadInfo). Please create channel using NetUtil.newChannel()
Somehow all of the cache info suddenly appeared.   Wonder if having the Browser Console open caused this?
(In reply to Matthew N. [:MattN] (behind on reviews) from comment #2)
> Is there an error in the Browser Console?

No error.
If I do a refresh of the page everything appears OK.  Even when listing the cache entries I don't get them.  Doing a refresh will make them appear.  Has nothing to do with the Browser Console.
Hm, the call to _isURILoadedFromFile is set using NetUtil.newChannel, which doesn't match up with what's seen in Comment 3.  I also don't see the error message in Comment 3
I've confirmed that commenting out these lines fixes the problem: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#7006-7019.  But why?  It's pretty much all wrapped up in a try/catch
This patch fixes the problem but I don't understand why
I guess the channel opened by the frontend [0] to determine if the page is a file is somehow interacting with the channel created to display about:cache [1].

[0]: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#7014
[1]: https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/nsAboutCache.cpp#31
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160407164938

I have tested your issue on latest FF release (45.0.2), latest Aurora(47.0a2), latest Nightly build (20160424030601) and managed to reproduce it, but only on the latest Nightly(48.0a1). I have opened about:support and after that I've opened about:cache in the same tab. There is no error in the console, and the cache data is displayed only after I refresh the page. This is reproducible regardless if e10s is enables or not.
Component: Untriaged → Networking: Cache
Product: Firefox → Core
Component: Networking: Cache → Security
Product: Core → Firefox
Easier STR:

* In any tab, load about:cache

Expected: it works
Actual: no entries show up.  A refresh fixes it

This needs further debugging around Comment 11 / https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/nsAboutCache.cpp#31.  We could take on a fix as in Comment 9, which works around the problem by not hitting this condition anymore for about: pages, but it'd be best to more fully understand what's wrong.
Whiteboard: [fxprivacy][triage]
Flags: qe-verify?
Priority: -- → P3
Whiteboard: [fxprivacy][triage] → [fxprivacy]
Hey Brian,

fxprivacy team doesn't have the bandwidth to look into this because of the notifications work.  So unless you have time for it or someone else volunteers, I think we shoudl back out https://bugzilla.mozilla.org/show_bug.cgi?id=1262009 so that about:cache works.
Flags: needinfo?(tanvi)
(In reply to Tanvi Vyas [:tanvi] from comment #14)
> Hey Brian,
> 
> fxprivacy team doesn't have the bandwidth to look into this because of the
> notifications work.  So unless you have time for it or someone else
> volunteers, I think we shoudl back out
> https://bugzilla.mozilla.org/show_bug.cgi?id=1262009 so that about:cache
> works.

Apparently backing out 1262009 will then cause bug 1245571 to fail.  I have a patch that fixes about:cache by not hitting the Channel creation code in the frontend (restoring the previous behavior which didn't hit this condition for about pages).  It's papering over what might be an issue with channel creation, but OTOH so is doing a backout.  So I guess I'll request review for this.
Flags: needinfo?(bgrinstead)
I some more debugging - this function: https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/nsAboutCache.cpp#31 is called immediately by the call to NetUtil.newChannel here https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#7019.

This can be confirmed by loading about:cache in the URL bar and then running this in the browser console:

var chanOptions = {uri: gBrowser.currentURI, loadUsingSystemPrincipal: true};
NetUtil.newChannel(chanOptions).URI.spec;

And the code in nsAboutCache is called each time.  So, it's called twice when loading the URL the first time, and I see these warnings in the output:

###!!! [Child][MessageChannel] Error: (msgtype=0xF60001,name=PTexture::Msg___delete__) Channel error: cannot send/recv
###!!! [Child][OnMaybeDequeueOne] Error: Channel error: cannot send/recv
###!!! [Child][OnMaybeDequeueOne] Error: Channel error: cannot send/recv
###!!! [Child][OnMaybeDequeueOne] Error: Channel error: cannot send/recv

Tanvi: do you know if it's expected that calling NetUtil.newChannel will 'load' that page (even though the channel is never opened and we are using it to just get the resolved URI)?
Flags: needinfo?(tanvi)
Flags: qe-verify? → qe-verify+
(In reply to Brian Grinstead [:bgrins] from comment #16)
> I some more debugging - this function:
> https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/
> nsAboutCache.cpp#31 is called immediately by the call to NetUtil.newChannel
> here
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> js#7019.
> 
> This can be confirmed by loading about:cache in the URL bar and then running
> this in the browser console:
> 
> var chanOptions = {uri: gBrowser.currentURI, loadUsingSystemPrincipal: true};
> NetUtil.newChannel(chanOptions).URI.spec;
> 
> And the code in nsAboutCache is called each time.  So, it's called twice
> when loading the URL the first time, and I see these warnings in the output:
> 
> ###!!! [Child][MessageChannel] Error:
> (msgtype=0xF60001,name=PTexture::Msg___delete__) Channel error: cannot
> send/recv
> ###!!! [Child][OnMaybeDequeueOne] Error: Channel error: cannot send/recv
> ###!!! [Child][OnMaybeDequeueOne] Error: Channel error: cannot send/recv
> ###!!! [Child][OnMaybeDequeueOne] Error: Channel error: cannot send/recv
> 
> Tanvi: do you know if it's expected that calling NetUtil.newChannel will
> 'load' that page (even though the channel is never opened and we are using
> it to just get the resolved URI)?

Passing this to Honza
Flags: needinfo?(tanvi) → needinfo?(honzab.moz)
Seems like opening the channel twice causes the problem here?  

I believe the problem is in how nsAboutCache is implemented, it's actually a singleton (there is a singleton buffer we write to.)  I'll fix it, should be quick.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Component: Security → Networking: Cache
Flags: needinfo?(honzab.moz)
Product: Firefox → Core
Whiteboard: [fxprivacy] → [fxprivacy][necko-active]
can this cause bug 1268536?
(In reply to Dragana Damjanovic [:dragana] from comment #19)
> can this cause bug 1268536?

Potentially, nsAboutCacheEntry suffers from the same issue, I'll fix it as well in this bug.  I think you can duplicate it.
Attached patch v1 (obsolete) — Splinter Review
Simply changes the behavior of nsAboutCache[Entry] from being singleton-like to a correct per-channel like behavior.

-w patch follows.
Attachment #8743531 - Attachment is obsolete: true
Attachment #8747688 - Flags: review?(michal.novotny)
Attached patch v1 -w (obsolete) — Splinter Review
(much simpler to review)
Summary: about:cache would not load properly if open it in the tab of about:home and any other about: page → about:cache channels cannot be instantiated in parallel, make it multi-instance capable
Tracking, regression from 48.
Blocks: 1271701
Attachment #8747688 - Flags: review?(michal.novotny) → review+
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5d40fc8b3bc9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Iteration: --- → 49.2 - May 23
Priority: P3 → P1
QA Contact: paul.silaghi
Verified fixed FX 49.0a1 (2016-05-22) Win 7
Status: RESOLVED → VERIFIED
The regression is from 48; do you want to request uplift to 48 aurora?
Flags: needinfo?(honzab.moz)
I think yes.  Thanks for a reminder.
Flags: needinfo?(honzab.moz)
Comment on attachment 8753343 [details] [diff] [review]
v1.0.1 (fixed build issues)

Approval Request Comment
[Feature/regressing bug #]: new HTTP cache + bug 1262009
[User impact if declined]: about:cache pages not rendered
[Describe test coverage new/current, TreeHerder]: few days on m-c, no idea if there are automated tests for this (probably not)
[Risks and why]: low, the change is actually pretty simple and well encapsulated
[String/UUID change made/needed]: none
Attachment #8753343 - Flags: approval-mozilla-aurora?
(In reply to Honza Bambas (:mayhemer) from comment #32)
> Comment on attachment 8753343 [details] [diff] [review]
> v1.0.1 (fixed build issues)
> 
> Approval Request Comment
> [Feature/regressing bug #]: new HTTP cache + bug 1262009

Or bug 1195757, not sure which one it actually causes.
Comment on attachment 8753343 [details] [diff] [review]
v1.0.1 (fixed build issues)

Fix a regression, taking it
Attachment #8753343 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
has problems to apply to aurora:

grafting 345461:5d40fc8b3bc9 "Bug 1266196 - Make about:cache handler be multiple-instance capable. r=michal"
merging netwerk/protocol/about/nsAboutCache.cpp
warning: conflicts while merging netwerk/protocol/about/nsAboutCache.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(honzab.moz)
(In reply to Carsten Book [:Tomcat] from comment #35)
> has problems to apply to aurora:
> 
> grafting 345461:5d40fc8b3bc9 "Bug 1266196 - Make about:cache handler be
> multiple-instance capable. r=michal"
> merging netwerk/protocol/about/nsAboutCache.cpp
> warning: conflicts while merging netwerk/protocol/about/nsAboutCache.cpp!
> (edit, then use 'hg resolve --mark')
> abort: unresolved conflicts, can't continue
> (use 'hg resolve' and 'hg graft --continue')

The reason is that to apply cleanly (not to function!) it needs to be applied over [1] - bug 1268313.

I can provide a cleanly applying patch, but you will have issues when merging m-c to m-a.  If that is not an issue, I will submit the m-a patch.

[1] https://hg.mozilla.org/mozilla-central/diff/114ca1fc9c51/netwerk/protocol/about/nsAboutCache.cpp#l182
Flags: needinfo?(honzab.moz) → needinfo?(cbook)
Attached patch v1.0.1 for m-aSplinter Review
Flags: needinfo?(cbook)
Verified fixed FX 48.0a2 (2016-05-31) Win 7
You need to log in before you can comment on or make changes to this bug.