281922 - [FIXr]iframe document region is not updated in this testcase

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Martijn Wargers (dead)]

This might very well be the same bug as bug 173858 (or have the same cause), but
the testcase I'll post here is somewhat different. In this testcase there is not
a dependancy on xbl.
You need to have Adblock installed to see this bug. It doesn't matter if you
have it disabled or enabled, the bug is still showing.

The problem is is that in this testcase the document region doesn't seem to get
the height of the iframe (which is dynamically set to 500px). Instead of that,
the document seems to stay at the default height of 90px. 

This bug has the same regression period as bug 173858.

Mozilla seems to 'forget' to paint the area between the iframe document and the
iframe. Every thing that is overlayed over that area and then removed 'sticks'
at that area.

Comment 1

[Martijn Wargers (dead)]

Argh, sorry, screw-up of me. I can also see the bug in Mozilla1.7. However, the
'dead' area gets updated there.

Comment 2

[Martijn Wargers (dead)]

Attachment: iframe for testcase

Comment 3

[Martijn Wargers (dead)]

Attachment: testcase

And I see, I've also mentioned the wrong bug in comment 0. I meant bug 281657.
But this has probably nothing to do with bug 281657, in hindsight, see comment
1.

Comment 4

[Martijn Wargers (dead)]

This is the regression period of the 'Mozilla "forgets" to paint this area' part
of the bug:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/20041010 6:49am
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/20041012
Firefox/0.9.1+ 7:18am
This is in the period where bug 238493 was fixed.

The original bug, I can see also in Mozilla1.4. Before that, I can't test,
because Adblock doesn't seem to work in older builds.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Does disabling adblock still apply its XBL binding (check in DOM inspector)?  If
so, does just applying that binding to the object/embed (both?) cause the problem?

Comment 6

[Martijn Wargers (dead)]

I'm not seeing any xbl binding applied with Adblock in DOM Inspector, neither
with Adblock enabled or disabled.
I think I'll try to tear down Adblock to see what it is doing when turned off.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Maybe I'm confusing adblock and flashblock....  Does adblock just toss lots of
rules into userContent.css?

Comment 8

[Martijn Wargers (dead)]

Flashblock adds xbl bindings to object tags and so (and css rules in
userContent.css).
Adblock doesn't add rules in userContent.css. I don't know how Adblock works, it
seems very complicated.

Comment 9

[Ben Karel [eschew]]

(In reply to comment #7)
> Maybe I'm confusing adblock and flashblock....  Does adblock just toss lots of
> rules into userContent.css?

The current version of Adblock (0.5) doesn't use userContent.css. When a given
piece of content matches a user-entered regexp, Adblock (optionally) returns
nsIContentPolicy::REJECT_REQUEST, and then sets 'display: none' or 'visibility:
hidden' on the style property of the blocked node.

0.6 will use a few (~7?) rules in userContent.css to let us just add an
attribute to the node and allow CSS to do the hiding. (Mainly to make unblocking
easier, avoiding loss of local style information.) No XBL bindings in
userContent, just display: none and other "basic" properties.


If Adblock is disabled in the Extension Manager, then we might as well be
uninstalled -- no XBL, no JS, no nothing. However, the only difference between
normal operation and being "disabled" (greyed-out statusbar label) is that we
return early from ::shouldLoad().

Martijin: Look at the top level of any chrome/browser window. We add an
<adblock> element and attach a simple binding that adds a 'load' event-listener
if it's the first window opened; that listener in turn initializes Adblock and
prevents further listeners from being registered.

Comment 10

[Martijn Wargers (dead)]

Well, removing this rule in component.js of Adblock:
compMgr.registerFactory(cid, CONTENTPOLICY_DESCRIPTION,
CONTENTPOLICY_CONTRACTID, factory);
makes the bug go away.
(const CONTENTPOLICY_CONTRACTID = "@mozilla.org/layout/content-policy;1";
const CONTENTPOLICY_DESCRIPTION = "Content policy service";)

Maybe this is just a bug in Adblock, then?
I have no idea what this kind of thing is doing and I'm not capable of making a
testcase with that code. Running that code in the example is doing weird stuff
with my Firefox build.

Comment 11

[Ben Karel [eschew]]

(In reply to comment #10)
> Well, removing this rule in component.js of Adblock:
> compMgr.registerFactory(cid, CONTENTPOLICY_DESCRIPTION,
> CONTENTPOLICY_CONTRACTID, factory);
> makes the bug go away.
> (const CONTENTPOLICY_CONTRACTID = "@mozilla.org/layout/content-policy;1";
> const CONTENTPOLICY_DESCRIPTION = "Content policy service";)
> 
> Maybe this is just a bug in Adblock, then?
> I have no idea what this kind of thing is doing and I'm not capable of making a
> testcase with that code. Running that code in the example is doing weird stuff
> with my Firefox build.

Instead of "properly" registering ourself as an additional link in the
nsIContentPolicy chain, we actually replace the default contentpolicy module.
It's a hack, but apparently it's the only way rue and Henrik have found to
maintain backwards-compatibility to Mozilla 1.0 (not sure why < 1.4 didn't work
for you, as 1.0 wfm; i've never bothered testing farther back).

I'm not sure why this is causing problems here, as we keep the replaced module
around to give us a default return value to pass back to the browser should no
user filters trigger a match, and anyways the content policy shouldn't be called
on a reflow event....

Comment 12

[Boris Zbarsky [:bzbarsky]]

Ben, is there some URL where I can read the adblock content policy
implementation code?  

Comment 13

[Ben Karel [eschew]]

(In reply to comment #12)
> Ben, is there some URL where I can read the adblock content policy
> implementation code?  

Well, CVS checkins stopped 20 months ago (before I came on board), but you can
browse the plaintext here: 

http://eschew.org/projects/adblock/old/source/

component.js is the file you're likely to be most interested in; try searching
for  origComponent to get to the already-mentioned registration bits. For the
actual implementation, search for shouldLoad. I can tell you, though, that
shouldLoad's implementation doesn't matter a bit; you can have it just return 1
(ACCEPT) and it'll still display the bug.

Comment 14

[Boris Zbarsky [:bzbarsky]]

So let me see whether I understand this right:

1)  When Adblock is disabled the bug is present
2)  When Adblock is disabled, all that happens is the shouldLoad calls through to
    the existing impl.
3)  There are no other Adblock hooks.

If so, sounds like I'll have to install Adblock (URI?) and try to sort out
what's going on...  I did read the code, and it seems like it should work fine.  :(

Comment 15

[Ben Karel [eschew]]

(In reply to comment #14)
> So let me see whether I understand this right:
> 
> 1)  When Adblock is disabled the bug is present
> 2)  When Adblock is disabled, all that happens is the shouldLoad calls through to
>     the existing impl.
> 3)  There are no other Adblock hooks.
> 
> If so, sounds like I'll have to install Adblock (URI?) and try to sort out
> what's going on...  I did read the code, and it seems like it should work
fine.  :(

1) Yes. (Disabled not through the Extension Manager but through Adblock itself).
2) Not exactly, but as I said, the implementation doesn't matter: you can edit
shouldLoad() so that the first line is "return 1;" and the bug is still present.
The only other ContentPolicy function we implement is shouldProcess, and that
*is* just a pass-through (and it's not being called).
3) Hooks into what, exactly? 0.5 doesn't do any other XPCOM-trickery, if that's
what you meant. And yeah, it generally works fine. It might be interesting to
see what happens if Adblock is registered "properly" as another link in the
contentpolicy chain, instead of as the default module's replacement.


Install XPI located at http://downloads.mozdev.org/adblock/adblock-0.5-dev.xpi

Comment 16

[Boris Zbarsky [:bzbarsky]]

> 3) Hooks into what, exactly?

Into any aspect of the page rendering.

I'll try to locate a tree with XPInstall actually working when I get back into
town so I can try this.....

Comment 17

[Ben Karel [eschew]]

(In reply to comment #16)
> > 3) Hooks into what, exactly?
> 
> Into any aspect of the page rendering.
> 
> I'll try to locate a tree with XPInstall actually working when I get back into
> town so I can try this.....

I can't think of anything else we do outside of shouldLoad's modifications
(which, as we've established, have no relevance here.)

I don't recall offhand whether 0.5 works with the trunk; afaik some trunk builds
work and some don't. If working with the trunk is a necessity, I'll try to hook
you up with a trunk-compatible Adblock; otherwise, the bug is, as Martijn noted,
present back to Moz 1.4, and probably earlier (window.frameElement is not
defined in 1.0).

Comment 18

[Boris Zbarsky [:bzbarsky]]

Working with trunk is sorta a necessity, since I don't have any non-trunk debug
trees and don't really plan to have any...

Comment 19

[Boris Zbarsky [:bzbarsky]]

OK, I poked at this.  This is somehow getting confused because of the
reflow-flushing that has to happen for <object> XPCWrappedNative creation...
This is causing us to do all sorts of fun stuff...  In any case, bug 1156 will
make all this better as far as plugins are concerned.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch that fixes this bug for me

The comments should make it pretty clear what's going on here...

Comment 21

[Martijn Wargers (dead)]

Ok, I've tested the patch. Before the patch I see the bug in my debug build,
after I've applied the patch, I don't see the bug anymore.

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 189212 [details] [diff] [review]
Patch that fixes this bug for me

r+sr=jst

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 189212 [details] [diff] [review]
Patch that fixes this bug for me

Requesting 1.8b4 approval.  This is pretty safe and makes sense in general as
extra protection.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Fixed.