336875 - With Adblock Plus 0.7 installed, images disappear upon reloading any page

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Ria Klaassen (not reading all bugmail)]

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.

Comment 1

[Ria Klaassen (not reading all bugmail)]

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

Comment 2

[Ria Klaassen (not reading all bugmail)]

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

Comment 3

[Ria Klaassen (not reading all bugmail)]

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

Comment 4

[Ria Klaassen (not reading all bugmail)]

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

Comment 5

[Ria Klaassen (not reading all bugmail)]

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

Comment 6

[Ria Klaassen (not reading all bugmail)]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

[Wladimir Palant]

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.

Comment 9

[Peter van der Woude [:Peter6]]

That would be Bug 336785
cc Brettw

Comment 10

[Wladimir Palant]

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.

Comment 11

[Peter van der Woude [:Peter6]]

(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 

Comment 12

[Boris Zbarsky [:bzbarsky]]

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?

Comment 13

[Wladimir Palant]

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.

Comment 14

[Wladimir Palant]

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.

Comment 15

[Wladimir Palant]

Attachment: Minimized content policy

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.

Comment 16

[Wladimir Palant]

Attachment: Minimized testcase

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.

Comment 17

[Wladimir Palant]

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

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

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

(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 :-/.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 24

[Boris Zbarsky [:bzbarsky]]

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.

Comment 25

[Boris Zbarsky [:bzbarsky]]

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.

Comment 26

[Boris Zbarsky [:bzbarsky]]

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

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Possible fix

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.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Without parentStr

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.

Comment 29

[Tracy Walker [:tracy]]

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.

Comment 30

[Boris Zbarsky [:bzbarsky]]

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.

Comment 31

[Tracy Walker [:tracy]]

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.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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

Comment 33

[Brendan Eich [:brendan]]

Attachment: proposed fix to js_CheckAccess

diff -w version next.

/be

Comment 34

[Brendan Eich [:brendan]]

Attachment: Blake caught a preexisting bug, and a few nits

Comment 35

[Brendan Eich [:brendan]]

Attachment: diff -w version of last attachment

Easier to review.

/be

Comment 36

[Blake Kaplan (:mrbkap) (inactive)]

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

Thanks, r=mrbkap

Comment 37

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Best fix yet

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

Comment 38

[Tim Riley [:timr]]

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.

Comment 39

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 40

[Tim Riley [:timr]]

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

Comment 41

[gabe]

check this in on trunk it is annoying

Comment 42

[Ryan VanderMeulen [:RyanVM]]

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

Comment 43

[Régis Caspar]

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

Comment 44

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk and the 1.8 branch.

Comment 45

[Ria Klaassen (not reading all bugmail)]

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

Comment 46

[Loki]

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.

Comment 47

[Dave Townsend [:mossop]]

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

Fine here.

Comment 48

[Loki]

(In reply to comment #47)
> Fine here.

Sorry. Tried with a clean profile and AdBlock installed - works perfectly. My bad.
 

Comment 49

[Alexander Sack]

Attachment: 1.0.x backport

needed for 1.0.9+ / 1.7.14+