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)
Tracking
()
People
(Reporter: alice0775, Assigned: mayhemer)
References
Details
(Keywords: regression, Whiteboard: [fxprivacy][necko-active])
Attachments
(2 files, 3 obsolete files)
27.62 KB,
patch
|
mayhemer
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
27.63 KB,
patch
|
Details | Diff | Splinter Review |
[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)
Comment hidden (obsolete) |
Comment 3•8 years ago
|
||
(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()
Comment 4•8 years ago
|
||
Somehow all of the cache info suddenly appeared. Wonder if having the Browser Console open caused this?
Reporter | ||
Comment 5•8 years ago
|
||
(In reply to Matthew N. [:MattN] (behind on reviews) from comment #2) > Is there an error in the Browser Console? No error.
Comment 6•8 years ago
|
||
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.
Comment 7•8 years ago
|
||
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
Comment 8•8 years ago
|
||
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
Comment 9•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/47867/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47867/
Comment 10•8 years ago
|
||
This patch fixes the problem but I don't understand why
Comment 11•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Component: Networking: Cache → Security
Product: Core → Firefox
Comment 13•8 years ago
|
||
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]
Updated•8 years ago
|
Flags: qe-verify?
Priority: -- → P3
Whiteboard: [fxprivacy][triage] → [fxprivacy]
Comment 14•8 years ago
|
||
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)
Comment 15•8 years ago
|
||
(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)
Comment 16•8 years ago
|
||
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)
Updated•8 years ago
|
Flags: qe-verify? → qe-verify+
Comment 17•8 years ago
|
||
(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)
Assignee | ||
Comment 18•8 years ago
|
||
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]
Comment 19•8 years ago
|
||
can this cause bug 1268536?
Assignee | ||
Comment 20•8 years ago
|
||
(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.
Assignee | ||
Comment 22•8 years ago
|
||
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)
Assignee | ||
Comment 23•8 years ago
|
||
(much simpler to review)
Assignee | ||
Updated•8 years ago
|
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
Comment 24•8 years ago
|
||
Tracking, regression from 48.
Updated•8 years ago
|
Attachment #8747688 -
Flags: review?(michal.novotny) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment hidden (obsolete) |
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 26•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5a6263bbdfd
Attachment #8747688 -
Attachment is obsolete: true
Attachment #8747689 -
Attachment is obsolete: true
Attachment #8753343 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 27•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d40fc8b3bc9
Keywords: checkin-needed
Comment 28•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5d40fc8b3bc9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Updated•8 years ago
|
Iteration: --- → 49.2 - May 23
Priority: P3 → P1
Updated•8 years ago
|
QA Contact: paul.silaghi
Comment 30•8 years ago
|
||
The regression is from 48; do you want to request uplift to 48 aurora?
Flags: needinfo?(honzab.moz)
Assignee | ||
Comment 32•8 years ago
|
||
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?
Assignee | ||
Comment 33•8 years ago
|
||
(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 34•8 years ago
|
||
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+
Comment 35•8 years ago
|
||
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)
Assignee | ||
Comment 36•8 years ago
|
||
(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)
Assignee | ||
Comment 37•8 years ago
|
||
Updated•8 years ago
|
Flags: needinfo?(cbook)
Comment 38•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/db6a952a98f8
Comment 39•8 years ago
|
||
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.
Description
•