Steps to reproduce: 1. Install Adblock Plus and go to http://www.nu.nl/ 2. Reload 3. Result: no images. Regression between 1.9a1_2006050513 and 1.9a1_2006050516.

Bonsai link: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-05-05+12%3A00&maxdate=2006-05-05+16%3A00

Sometimes the image shows up on mouseover, for instance the NU logo on top.

And sorry if it's the wrong component, I am not a programmer but a simple tester so I can only guess.

Hovering only works when there is replacing text for the image. Shift+reload works as expected.

Adblock Plus extension: https://addons.mozilla.org/firefox/1865/

I see it also on this page http://forum.fok.nl/topic/844183/1/50 but not on this one http://images.google.com/images?q=puppy&hl=en&btnG=Search+Images

This isn't the first time a bug like this has happened. See Bug 309076.

Confirming regression (tested in 1.9a1_2006050405 and 1.9a1_2006050604). Tested it with a simplistic content policy instead of Adblock Plus - the problem disappears, so that's not the reason. However, disabling Adblock Plus (meaning that it won't block anything but still track the objects loaded) doesn't make the problem go away. I suspect that the regression is caused by the changes in nsEventListenerManager - Adblock Plus uses event handlers to bind its data to a document.

That would be Bug 336785 cc Brettw

No, there are three nsEventListenerManager checkins all from different bugs. I think bug 329192 is more likely to be the reason, I'll have it checked in an hour.

(In reply to comment #10) > No, there are three nsEventListenerManager checkins all from different bugs. I > think bug 329192 is more likely to be the reason, I'll have it checked in an > hour. > You're right. This isn't a branch problem which rules out Bug 336785

I'll try to debug this tomorrow, but if this happens on some sites and not others, could someone try to create a minimal-ish testcase? Also, I just tried installing Adblock Plus 0.7 in a current trunk Firefox build, and it installed but was disabled after restart (something about being not compatible)... Anyone know what's up with that?

Boris, do you mean that it was disabled in the extension manager? I also had weird behavior from the extension manager, I had to remove extensions.rdf to enable Adblock Plus (the nightly that is only a few hours older works fine). Took me longer (had to upgrade my build environment) but I backed out the patch from bug 329192 now - nothing changed. It is something else then.

Replacing js3250.dll in 1.9a1_2006050516 by the one from 1.9a1_2006050513 makes the problem go away. I don't have time right now to back out the patch properly but I think the checkin in question is bug 336601.

The line of code in Adblock Plus 0.7 that makes images fail to load is node.ownerDocument.defaultView. It is enough to call this for the logo image nu_logo.gif and for the script misc.js, the logo image will not load on refresh then. Attaching minimized content policy object that allows to reproduce the problem without Adblock Plus installed. Simply save it as TestPolicy.js and drop it into the application's components directory. Then remove compreg.dat in your profile to make the component register. Doing the same thing *after* the page loaded doesn't seem to have any effect.

In fact, misc.js and nu_logo.gif are the only two elements that need to be on the page (and the DOCTYPE declaration for some reason). Attaching minimal testcase.

Tested a little further: misc.js can be empty, its contents don't matter obviously.

Wladimir, that content policy implementation violates the nsIContentPolicy API documentation, so I would fully expect things to break with it. See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/public/nsIContentPolicy.idl&rev=3.8&mark=189-195#180 Changing the content policy to do: (new XPCNativeWrapper(insecNode)).ownerDocument.defaultView; makes the testcase work for me... I'll try adblock next, I guess; I thought they did use XPCNativeWrapper...

Wladimir, thanks for the minimal HTML testcase! OK, so here's the pseudo-stack to how this busts (with some not-really-relevant frames removed): nsHTMLDocument::FlushPendingNotifications ... nsHTMLDocumentSH::NewResolve XPC_WN_Helper_NewResolve js_LookupPropertyWithFlags js_CheckAccess js_ComputeThis js_Invoke js_Interpret js_Invoke nsXPCWrappedJSClass::CallMethod <-- Call into adblock ... nsContentPolicy::ShouldLoad nsContentUtils::CanLoadImage The property we're resolving, unsurprisingly, is __parent__. The adblock code where we hit this is: // Retrieves the value of a property but ignores any redefined properties // Usage: secureGet(object, "property1", "property2", ...) // This is a secure version of object.property1.property2 function secureGet() { var method = secureLookup.apply(null, arguments); try { return (method && method.arity == 0 ? method() : null); } catch (e) { return null; } } It's being called as: secureGet(insecNode, "nodeType") as far as I can tell. So at a guess, this is a regression from bug 336601 (and the bonsai range in comment 1 ends 30 minutes too early). I'm a little confused about how the JS engine comes to be working with the unwrapped document JSObject here. But given that it is, the rest sorta works like it should, since <form name="__parent__"> does override document.__parent__ in Gecko. I should note that the fact that it does so probably means that this code really shouldn't be getting the __parent__ property...

Oh, so the reason we're resolving the __parent__ property is that OBJ_CHECK_ACCESS() does that: (gdb) frame 8 #8 0xb7ef468f in js_ComputeThis (cx=0x84419a0, thisp=0x8c67070, argv=0x91dcf08) at ../../../mozilla/js/src/jsinterp.c:527 527 if (!OBJ_CHECK_ACCESS(cx, thisp, id, JSACC_PARENT, &v, &attrs)) (gdb) frame 7 #7 0xb7f1fd09 in js_CheckAccess (cx=0x84419a0, obj=0x8c67070, id=135096848, mode=JSACC_PARENT, vp=0xbfffd0ac, attrsp=0xbfffd0a8) at ../../../mozilla/js/src/jsobj.c:3577 3577 if (!js_LookupProperty(cx, obj, id, &pobj, &prop)) I'm still confused how we got our hands on the underlying XPCWrappedNative here. We should be working with XPCNativeWrappers....

To be precise, the |thisp| passed to js_ComputeThis is an XPCWrappedNative, not an XPCNativeWrapper.

(In reply to comment #21) > To be precise, the |thisp| passed to js_ComputeThis is an XPCWrappedNative, not > an XPCNativeWrapper. That can't be right. The |thisp| passed to js_ComputeThis must be the Call (activation) object for some function or null. Do you know what function we're trying to call? Also, why is this failing? Does the security check fail, or is this happening at a bad time or what? I'm not sure what the JS engine will be able to do about this :-/.

> That can't be right. Ok, true. I guess js_ComputeThis assigns to |thisp|. I'll breakpoint in it and see what's actually going on. > Also, why is this failing? It fails because it resolves a property on an HTMLDocument, which flushes the content sink while the content sink is in the middle of appending a node to the DOM. The node assumes that this sort of thing can't happen, and we lose a ContentStatesChanged notification. I could always fire the ContentStatesChanged notification, even when I think we don't need it, but that would regress pageload a bunch. And the other part of what I said is that if the security check depends on the value of document.__parent__ (not to be confused with what JS_GetParent returns!) and HTML pages can override that value, then the security check has a problem, no?

OK, so indeed thisp is null when js_ComputeThis is called. The function we're trying to call is presumably the return value of Components.utils.lookupMethod on the document's XPCNativeWrapper.

OK. There are no XPCNativeWrappers involved.... In fact, our XPCNativeWrapper automation really kinda fails in this case. What happens is that we're calling from C++ out to JS, passing out an object. So in XPCConvert::NativeInterface2JSObject we have ccx.mJSContext->fp null, since we haven't called into JS yet! In fact, if it happened that there _was_ JS on stack (eg content JS inserting an image node which would then call out to content policy), we'd use the wrong filename for the flags... We should probably get a separate bug filed on this, eh? Be that as it may, the upshot is that the component is NOT using XPCNativeWrappers. Instead it seems to be using Components.lookupMethod, which used to have a similar effect from the point of view of not resolving random stuff on HTMLDocuments. But the patch for bug 336601 changed that. So now we have an HTMLDocument, we call Components.utils.lookupMethod on it, then call that method, which resolves our |this|, which makes us walk the parent chain of the HTMLDocument, and things break.

I filed bug 337095 on the XPCNativeWrapper issues, since they're sorta orthogonal to this bug.

This patch relies on the fact that the default checkObjectAccess hook doesn't modify the value of vp. I'm not sure if that's a problem. This patch should fix the problem described here by avoiding the js_LookupProperty found in js_CheckAccess. This is the most painless way I can think of to solve this bug.

I decided against the single-use parentStr to extract the jsval out of parentAtom and then forgot to copy the patch to the proper place.

bz, at the given test page www.nu.nl, on Windows XP, I found that the banner ad at the top of the page is not displayed with adblock installed and with it not installed. The banner ad appears on Mac and Linux (adblock or not). All 3 builds were Fx 1.5.0.4 from 20060506. Is that perhaps another bug? As I mentioned in email, I can't otherwise reproduce this bug on the 1.8.0 branch on Fx.

That sounds like an unrelated issue (probably browser-sniffing on the site). As I said in email, this bug's adblock symptoms are probably not visible on the 1.8.0 branch. I'm still worried by the security implications of testing whether we can access the __parent__ property, which is not the same as testing whether we can access the return value of JS_GetParent.

bz,marcia and I tested this on Bon Echo builds from this morning. We are also not seeing a problem with adblock plus 0.7 there.

This is a trunk (Gecko 1.9a1) issue, not a Bon Echo branch (Gecko 1.8.1) issue.

diff -w version next. /be

Easier to review. /be

Comment on attachment 221340 [details] [diff] [review] Blake caught a preexisting bug, and a few nits Thanks, r=mrbkap

This patch actually runs and does the right thing. Note the JSACC_WATCH -> JSACC_TYPEMASK change.

Comment on attachment 221378 [details] [diff] [review] Best fix yet I talked to Brendan and mrbkap. This is a serious regression and there are some concerns about completeness of the previous fix. We need this for 1.5.0.4. a=timr for drivers.

Fix checked into the 1.8.0 branch. I'll check into the 1.8 branch in a few. I can't check into the trunk currently because it's closed (d'oh!).

I forgot to plus the blocker nomination when I plus'ed the patch for 1.5.0.4. Just finishing the job...

check this in on trunk it is annoying

I've applied this patch to my local tree and can verify that it works perfectly.

*** Bug 338098 has been marked as a duplicate of this bug. ***

Fix checked into trunk and the 1.8 branch.

After the third reload the images are still present. Thanks for the fix!

Still seeing wrong behavior with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060517 Minefield/3.0a1 - Build ID: 2006051706 For example, I'm trying http://livejournal.com and after reload there are no images.

(In reply to comment #46) > For example, I'm trying http://livejournal.com and after reload there are no > images. Fine here.

(In reply to comment #47) > Fine here. Sorry. Tried with a clean profile and AdBlock installed - works perfectly. My bad.

needed for 1.0.9+ / 1.7.14+