84224 - Trying to open cookperm.txt for every image load

Status: RESOLVED FIXED

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

Description

[Simon Fraser [no longer active]]

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()

Comment 1

[Simon Fraser [no longer active]]

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.

Comment 2

[Judson Valeski]

gone. this was part of my checkin to 84162. sorry :-/.

Comment 3

[Judson Valeski]

_gone_

Comment 4

[Simon Fraser [no longer active]]

Reopening. I'm still seeing this.

Comment 5

[Simon Fraser [no longer active]]

-> morse. This is the image blocking code.

Comment 6

[Simon Fraser [no longer active]]

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().

Comment 7

[Paul Chen]

nav triage team:

Hey-zoos, this sucks, but won't get to this soon. Marking this p2 and mozilla1.0

Comment 8

[Simon Fraser [no longer active]]

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.

Comment 9

[Simon Fraser [no longer active]]

Attachment: nsImages.cpp patch

Comment 10

[Simon Fraser [no longer active]]

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).

Comment 11

[Stephen P. Morse]

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.

Comment 12

[Simon Fraser [no longer active]]

But why do we have to load the file at all if the user set prefs to accept all 
images?

Comment 13

[Stephen P. Morse]

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.

Comment 14

[Paul Chen]

nav triage team:

Forgot to add nsbeta1+

Comment 15

[Stephen P. Morse]

I have a patch that fixes this the correct way.  sfraser, please review.  cc'ing 
alecf for super-review.

Comment 16

[Stephen P. Morse]

Attachment: create datastructure even if cookperm.txt doesn't exist

Comment 17

[Alec Flett]

change that to NS_ERROR_OUT_OF_MEMORY and you've got yourself an sr=alecf

Comment 18

[Simon Fraser [no longer active]]

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().

Comment 19

[selmer (gone)]

Not holding 091 for this perf fix. -> 092

Comment 20

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 21

[Stephen P. Morse]

fix checked in.