Closed Bug 84224 Opened 23 years ago Closed 23 years ago

Trying to open cookperm.txt for every image load

Categories

(Core :: Graphics: Image Blocking, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla0.9.2

People

(Reporter: sfraser_bugs, Assigned: morse)

References

Details

(Keywords: perf)

Attachments

(2 files)

My windows build is trying to open cookperm.txt for every image load in the chrome, which seems suboptimal. Stack: PERMISSION_Read() line 432 IMAGE_CheckForPermission(const char * 0x04237f30, const char * 0x04235f30, int * 0x0012e2c8) line 171 + 5 bytes nsImgManager::ShouldLoad(nsImgManager * const 0x04222364, int 2, nsIURI * 0x04236110, nsISupports * 0x03d13794, nsIDOMWindow * 0x01318cd4, int * 0x0012e2c8) line 96 + 29 bytes nsContentPolicy::CheckPolicy(nsContentPolicy * const 0x04222650, int 0, int 2, nsIURI * 0x04236110, nsISupports * 0x03d13794, nsIDOMWindow * 0x01318cd4, int * 0x0012e2c8) line 139 + 43 bytes nsContentPolicy::ShouldLoad(nsContentPolicy * const 0x04222650, int 2, nsIURI * 0x04236110, nsISupports * 0x03d13794, nsIDOMWindow * 0x01318cd4, int * 0x0012e2c8) line 166 NS_CheckContentLoadPolicy(int 2, nsIURI * 0x04236110, nsISupports * 0x03d13794, nsIDOMWindow * 0x01318cd4, int * 0x0012e2c8) line 56 + 112 bytes nsPresContext::StartLoadImage(nsPresContext * const 0x03143bf0, const nsString & {...}, const unsigned int * 0x00000000, const nsSize * 0x00000000, nsIFrame * 0x02cf633c, unsigned int (nsIPresContext *, nsIFrameImageLoader *, nsIFrame *, void *, unsigned int)* 0x00000000, void * 0x00000000, void * 0x02cf633c, nsIFrameImageLoader * * 0x0012e3a0) line 1270 + 38 bytes nsCSSRendering::PaintBackground(nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, nsIFrame * 0x02cf633c, const nsRect & {...}, const nsRect & {...}, const nsStyleBackground & {...}, const nsStyleBorder & {...}, int 0, int 0) line 2172 + 80 bytes nsBoxFrame::Paint(nsBoxFrame * const 0x02cf633c, nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 1302 + 37 bytes nsBoxFrame::PaintChild(nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsIFrame * 0x02cf633c, nsFramePaintLayer eFramePaintLayer_Underlay) line 1388 nsBoxFrame::PaintChildren(nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 1524 nsBoxFrame::Paint(nsBoxFrame * const 0x02cf61a0, nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 1342 nsBoxFrame::PaintChild(nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsIFrame * 0x02cf61a0, nsFramePaintLayer eFramePaintLayer_Underlay) line 1388 nsBoxFrame::PaintChildren(nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 1524 nsBoxFrame::Paint(nsBoxFrame * const 0x02cacf58, nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 1342 nsBoxFrame::PaintChild(nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsIFrame * 0x02cacf58, nsFramePaintLayer eFramePaintLayer_Underlay) line 1388 nsBoxFrame::PaintChildren(nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 1524 nsBoxFrame::Paint(nsBoxFrame * const 0x02c9ee10, nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 1342 nsBoxFrame::PaintChild(nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsIFrame * 0x02c9ee10, nsFramePaintLayer eFramePaintLayer_Underlay) line 1388 nsBoxFrame::PaintChildren(nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 1524 nsBoxFrame::Paint(nsBoxFrame * const 0x02c9ed2c, nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 1342 nsBoxFrame::PaintChild(nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsIFrame * 0x02c9ed2c, nsFramePaintLayer eFramePaintLayer_Underlay) line 1388 nsBoxFrame::PaintChildren(nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 1524 nsBoxFrame::Paint(nsBoxFrame * const 0x02c9eb98, nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 1342 nsBoxFrame::PaintChild(nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsIFrame * 0x02c9eb98, nsFramePaintLayer eFramePaintLayer_Underlay) line 1388 nsBoxFrame::PaintChildren(nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 1524 nsBoxFrame::Paint(nsBoxFrame * const 0x02c80454, nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 1342 nsBoxFrame::PaintChild(nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsIFrame * 0x02c80454, nsFramePaintLayer eFramePaintLayer_Underlay) line 1388 nsBoxFrame::PaintChildren(nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 1524 nsBoxFrame::Paint(nsBoxFrame * const 0x02c801b8, nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 1342 nsRootBoxFrame::Paint(nsRootBoxFrame * const 0x02c801b8, nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 263 nsContainerFrame::PaintChild(nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsIFrame * 0x02c801b8, nsFramePaintLayer eFramePaintLayer_Underlay) line 229 nsContainerFrame::PaintChildren(nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 173 nsContainerFrame::Paint(nsContainerFrame * const 0x02c8017c, nsIPresContext * 0x03143bf0, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Underlay) line 155 PresShell::Paint(PresShell * const 0x03144c14, nsIView * 0x03143330, nsIRenderingContext & {...}, const nsRect & {...}) line 5247 + 34 bytes nsView::Paint(nsView * const 0x03143330, nsIRenderingContext & {...}, const nsRect & {...}, unsigned int 128, int & 268601941) line 275 nsViewManager::RenderDisplayListElement(DisplayListElement2 * 0x042343d0, nsIRenderingContext & {...}) line 1438 nsViewManager::RenderViews(nsIView * 0x03143330, nsIRenderingContext & {...}, const nsRect & {...}, int & 0) line 1363 nsViewManager::Refresh(nsIView * 0x03143330, nsIRenderingContext * 0x04233a50, const nsRect * 0x0012f65c, unsigned int 1) line 900 nsViewManager::DispatchEvent(nsViewManager * const 0x031434d0, nsGUIEvent * 0x0012f79c, nsEventStatus * 0x0012f6a0) line 1957 HandleEvent(nsGUIEvent * 0x0012f79c) line 68 nsWindow::DispatchEvent(nsWindow * const 0x031431f4, nsGUIEvent * 0x0012f79c, nsEventStatus & nsEventStatus_eIgnore) line 712 + 10 bytes nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f79c, nsEventStatus & nsEventStatus_eIgnore) line 738 nsWindow::OnPaint() line 4003 + 28 bytes nsWindow::ProcessMessage(unsigned int 15, unsigned int 0, long 0, long * 0x0012fbd0) line 2998 + 17 bytes nsWindow::WindowProc(HWND__ * 0x00310370, unsigned int 15, unsigned int 0, long 0) line 979 + 27 bytes USER32! 77e71ab7() USER32! 77e71a77()
Keywords: perf
This is valeski's fault; only my windows build shows this, because I picked up valeski's changes there. This is a pretty bad performance regression.
Assignee: morse → valeski
OS: Windows NT → All
Hardware: PC → All
gone. this was part of my checkin to 84162. sorry :-/.
_gone_
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Reopening. I'm still seeing this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: Trying to open cookperm.txt for every XUL image load → Trying to open cookperm.txt for every image load
-> morse. This is the image blocking code.
Assignee: valeski → morse
Status: REOPENED → NEW
For *every* image load, we are hitting code the cookie DLL for image blocking. This involves reading in the cookperm.txt file (if it exists) every time! This happens because the "imageblocker.enabled" pref is on by default: http://lxr.mozilla.org/seamonkey/source/modules/libpref/src/init/all.js#351 which blizzard changed as a purported fix for bug 35981. Even when the pref is on, and the user has set their prefs to accept all images, we hit cookperm.txt. Why don't we bail in the 'PERMISSION_Accept' case? This logic lives in IMAGE_CheckForPermission().
nav triage team: Hey-zoos, this sucks, but won't get to this soon. Marking this p2 and mozilla1.0
Priority: -- → P2
Target Milestone: --- → mozilla1.0
This is such an easy kill that is should be brought back to 0.9.2 I think. This bug causes an extra stat and/or file read for every image that we load (cached or not). Attaching a patch which needs to be checked for sanity, but fixes the most common case.
Attached patch nsImages.cpp patch (deleted) — Splinter Review
The patch adds a few lines which bail before the file read if your image loading prefs are set to accept all images (PERMISSION_Accept).
The patch that Simon attached would be only a band-aid. The real fix is for the permission module to keep track of the fact that it already tried to open cookperm and not try again. The logic in the permission module is to set up a datastructure the first time that a permission is needed and to read the contents of cookperm into that datastructure. On future requests for permission, a check is made for the existence of the datastructure, and if it exists then the cookperm file is not re-read. Apparently if cookperm doesn't exist, the datastructure doesn't get set up and so future requests for permissions will attempt to read cookperm again. The correct fix is to set up the datastructure whether or not cookperm exists.
But why do we have to load the file at all if the user set prefs to accept all images?
Probably because the permission module tests for the permission setting before it tests the pref setting. The ordering of these tests should not be a big deal except for the very first time when the cookperm file gets read in. And that should happen only once, if the logic was working properly.
nav triage team: Forgot to add nsbeta1+
Keywords: nsbeta1+
I have a patch that fixes this the correct way. sfraser, please review. cc'ing alecf for super-review.
Status: NEW → ASSIGNED
Target Milestone: mozilla1.0 → mozilla0.9.1
change that to NS_ERROR_OUT_OF_MEMORY and you've got yourself an sr=alecf
r=sfraser Opportunities for later optimization: Cache the "imageblocker.enabled" pref. Do early return if the pref is set to 'PERMISSION_Accept'. Cache the string bundle inside of CKutil_Localize().
Not holding 091 for this perf fix. -> 092
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Blocks: 83989
a= asa@mozilla.org for checkin to the trunk. (on behalf of drivers)
fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago23 years ago
Resolution: --- → FIXED
Component: Cookies → Image Blocking
QA Contact: tever → nobody
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: