294960 - Adblock-able items list is blank

Status: RESOLVED FIXED

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

Description

[Jaime Mitchell (use [e-mail] for email)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050520
Firefox/1.0+ ID:2005052010

Install Adblock from u.m.o

Go to slashdot (with filterset.g the adds are blocked - so adblock is working).

Click the Adblock status bar icon, no items are listed in the adblock-able items
list.

Following message appears on the JS console:

Error: wnd has no properties
Source File: chrome://adblock/content/filterall.js
Line: 281

There should be a list of elements which adblock can block and those that it is
blocking.

Comment 1

[Brendan Eich [:brendan]]

Attachment: fix for the frames[0] js errors

Partial fix, still have to conquer the tainting of extensions that do not opt
into xpcnativewrappers=yes via their chrome manifests via chrome XBL from our
packages that are marked that way.

/be

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 184143 [details] [diff] [review]
fix for the frames[0] js errors

bz proposed a better place to put the special case, which is coming up next.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: better fix

bz, how's this grab you?

/be

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 184144 [details] [diff] [review]
better fix

r+sr=bzbarsky

Comment 5

[Brendan Eich [:brendan]]

Patch landed.  On to the XBL issue.

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: complete fix, I hope -- testing needed

Please test this, all you who can.

/be

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 184148 [details] [diff] [review]
complete fix, I hope -- testing needed

>+// If one of our class hooks is ever called from a non-system script, bypass
>+// the hook by calling the same hook on our wrapped native, with obj reset to
>+// the wrapped native's flat JSObject, so the call parameter can be simply
>+//
>+//      getProperty(cx, obj, id, vp)

I'll fix this, it refers to older macro parameterization that I reworked to
support casting for JSCLASS_NEW_RESOLVE.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: attempt to forward all appropriate JSClass hooks from nw to wn

when the nw is used from non-system-flagged chrome script.  Why doesn't it
work?  It actually breaks adblock, so that clicking on the lower-right corner
widget gives this error:

JavaScript error: chrome://adblock/content/adblock.js, line 903:
document.getElementById("content").contentDocument.defaultView has no
properties

gdb is so broken for me, I should rest, revive, and resurrect my Windows build.
If anyone can try this patch and say why the last XPC_NW_NewResolve before the
above error doesn't resolve defaultView in the nsIDOMDocumentView interface on
the document object, I'd be grateful.  From my stepping around in gdb, being
shown the wrong line half the time, it looked like the member was found, but
was not "local" (was in an xpcwrappednativeproto).  I then tried to watch the
lookup code go up to the proto and find the member, but no luck.

Ulp, I think talking it through just pointed out the problem: the nw proto
chain does not match the wn chain -- in fact it lacks the
xpcwrappednativeproto!	More when I've recovered, unless bz beats me to it.

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: this fixes adblock -- yay!

The layering is such that forwarding JSClass hooks won't work, because while
XPCWrappedNative uses JSClass hooks, it also uses XPCWrappedNativeProto to
share code and data via prototype-based delegation.  That delegation happens
above the JSClass layer, in the JSObjectOps (OBJ_...() macro calls) layer.

/be

Comment 10

[Brendan Eich [:brendan]]

Attachment: with bz's comments fixed

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 184165 [details] [diff] [review]
with bz's comments fixed

Rockin'

Comment 12

[Brendan Eich [:brendan]]

Fix checked in -- thanks again to bz for useful discussion and review.

/be

Comment 13

[Boris Zbarsky [:bzbarsky]]

Brendan, it looks like this patch fixes the instanceof issue (with it, I get
instanceof working right).  Any idea why?

Comment 14

[Boris Zbarsky [:bzbarsky]]

The patch has a problem.  The following change:

-  if (deep != JSVAL_FALSE && !JSVAL_IS_PRIMITIVE(v)) {
+  if (deep == JSVAL_TRUE && !JSVAL_IS_PRIMITIVE(v)) {

is wrong, since XPCNativeWrapper::GetNewOrUsed never sets that slot to any value
at all.  Or more precisely, XPCNativeWrapper::GetNewOrUsed should set it to
JSVAL_TRUE.  If I do that, then the instanceof test fails again, but it was only
succeeding to start with because I was apparently getting unwrapped stuff coming
through...

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix that up

To test, just try adblock on the trunk Firefox default start page -- without
this change it shows no blockable items, with it it shows the image.

Brendan, feel free to land this if it looks good to you.

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 184173 [details] [diff] [review]
Fix that up

Thanks, bz -- checked in.

/be