Steps to reproduce: 1. Recent Firefox trunk build win32 or OSX. e.g. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060717 Minefield/3.0a1 2. Install latest Flashblock 1.5.unstable from <http://flashblock.mozdev.org/installation1.html> 3. Restart Browser. 4. Visit <http://www.youtube.com/watch?v=p4qOjLtB4tE&feature=Views&page=1&t=t&f=b> Flashblock has removed the flash object and replaced it with a placeholder DIV. Verified by DOMi but the original flash object continues to be visible above the flashblock placeholder. Looking at this page via DOMi, the running flash object can't be selected in the browser view using the Qpick button. In the DOM tree view the flash object has been removed from the DOM by flashblock and all I can see is the Flashblock placeholder. Note that this is trunk only. This problem does not occur in 1.8 or in 1.8.0 or in 1.7. Original mozdev bug: <http://bugzilla.mozdev.org/show_bug.cgi?id=13137>

This breaks a fairly popular Firefox extension (as well as part of Camino) on trunk/1.9....

Would be good if we could get a minimal testcase here that doesn't use all of flashblock. For all we know this might be a flashblock bug. Feel free to renominate once there is a testcase showing that this is indeed a firefox problem.

Not sure why I was CC'ed on this bug since I am by no means an expert on Flashblock. Anyway, I can make a minimized testcase. The testcase should be opened *without* Flashblock being installed. Flashblock binds its XBL to the Flash object and removes the object from the DOM tree in the XBL constructor. This testcase will only bind an empty binding to the Flash object - as can be seen it leads to a duplication of the object. One is visible in the DOM tree, that's the one Flashblock removes. The bogus one stays.

Forgot to mention an interesting observation: when Adblock Plus is installed this bug doesn't appear, even if Adblock Plus is disabled in its preferences. I guess the existence of a JavaScript-based content policy does this already.

One thing that's been bugging me. How can something that's visible in the browser window /not/ show up in the DOM tree view in the DOM Inspector? Phil

Jonas, please reconsider blocking1.9 again

As the original reporter, I am renominating blocking-1.9 as there is now a minimized test case. Thanks Wladimir!

Assigning to me for now. Could someone find a regression range for this?

The regression range is 2005-12-02 to 2005-12-03. This far back the inline data url doesn't work. I had to split the XBL out into a separate file

Regression Range between 2005120206 and 2005120306 using Win32 zip builds to test. Bonsai Query: <http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-12-02+00%3A00%3A00&maxdate=2005-12-03+06%3A46%3A00&cvsroot=%2Fcvsroot> Would this be Bug 315841?

bug 315841 also sounds like a good candidate. Biesi, could you take a look at this?

> How can something that's visible in the browser window /not/ show up in the DOM > tree view in the DOM Inspector? When we created two frames for the same node, then removed one of them with the node. The problem is that when the XBL binding is attached it JS-wraps the node (in case it has to install script stuff on it). That causes the classinfo code to create a frame for the node. But the XBL binding is attached at the very beginning of frame construction for the node, so we go ahead with said construction and end up with two frames. So we're sort of being screwed by the suck that is XBL, the suck that is classinfo for plugins, and the suck that is content policy callsites (which was the issue in bug 315841). More precisely, if creating a JS wrapper requires up-to-date frame state (as with plug-ins) and creating the frame requires a JS wrapper (as with XBL), then we lose. I don't see a way to have both work.

I guess we could make IsSafeToFlush() catch more cases (e.g. notify the presshell when we're processing frame construction off of restyle events) and then use it to bail out early from RecreateFramesForContent. Or something. We'd need that to not assert that we're reentering frame construction here anyway. David, have you had any thoughts along these lines?

How about we just move out the JS-wrapping, and in fact, all the script stuff XBL does, out to when we'd call the constructor? We need to do that for fields anyway.

jonas, do you want to own this bug perhaps? I don't really know much about XBL...

Ok, i'll give this a shot.

Targeting M9 as this bug will block beta.

After re-considering this, are we sure that we'd block beta because the flash blocker extension is broken? Not saying it isn't important. I think we should definitely keep it a blocker, but considering that we haven't made progress on this so far, and beta freeze is days away, I don't see this making beta. I don't think we should hold up beta for this. Any objections?

I have a partial patch, so I'd like to try to get in for beta.

> After re-considering this, are we sure that we'd block beta because the flash > blocker extension is broken? FYI: There are also two crash bugs that depend on this one.

This seems to fix it. I'm calling InstallImplementation and InstallEventHandlers mostly from the same place where we call ExecuteAttachedHandler rather than from LoadBindings directly. However, I was a little worried that someone would be able to get hold of the node between the point where the nsCSSFrameCtor adds the binding and the last EndUpdate causes InstallImplementation+InstallEventHandlers+ExecuteAttachedHandler to be called. In the old code we would have all methods and fields available, but with the new code that would not be the case. So I made nsElementSH::PostCreate call InstallImplementation+InstallEventHandlers if that happens. In theory PostCreate isn't an ideal hook since it won't be called if a binding is removed and another one is attached, but it seems to work good enough, especially since that's where we currently force ExecuteAttachedHandler to be called early sometimes.

Comment on attachment 285299 [details] [diff] [review] Patch to fix >Index: content/xbl/src/nsXBLBinding.h >+ // These two functions recursivly installs the eventhandlers "recursively install the event handlers" >Index: content/xbl/src/nsXBLService.cpp >+ *aBinding = newBinding; >+ NS_ADDREF(*aBinding); newBinding.swap(*aBinding)? And probably set *aResolveStyle before you do that, if you make this change? With those nits, r=bzbarsky. This looks great. Get some tests in too?

The problem was that the EnsureScriptAPI call in the PostCreate hook didn't get called since we bailed early if there's a frame there. Fixed by moving the check for binding to before checking for frames.

Comment on attachment 286352 [details] [diff] [review] Patch v2 Actually, this is a beta blocker, so no approval needed.

I don't think we can remove the IsSafeToFlush check, really. See bug 401155. Though given jst's further comments in that bug, maybe we're just screwed no matter what...

Checked in and passing tests. W00t!

I wonder whether we can add a test for this...

Could we add a reftest somehow? Do reftests and plugins work together?

Verified fixed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007102605 Minefield/3.0a9pre. Tested with: 1. Original URL 2. Wladimir's minimized test case. 3. Revised test case.

This has been verified via backout to be the cause of bug 401470.

Bug 401463 and bug 401503 have both been verified via backout as being caused by this check-in. That brings to 3 the number of places where this check-in broke parts of the Firefox UI.

I just filed the bug (https://bugzilla.mozilla.org/show_bug.cgi?id=401581) regarding this check-in.

(In reply to comment #30) > This has been verified via backout to be the cause of bug 401470. > (In reply to comment #31) > Bug 401463 and bug 401503 have both been verified via backout as being caused > by this check-in. > > That brings to 3 the number of places where this check-in broke parts of the > Firefox UI. > these regressions should be "blocking1.9+" and should be fixed before beta ?

This went off my radar because it was marked fixed until a Flashblock user complained again about Firefox 3.0RC1. Was this patch backed out or is it a regression? Duplicated Flash content (when Flashbock in installed) seen using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008052106 Minefield/3.0pre Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a1pre) Gecko/2008052103 Minefield/3.1a1pre Intrestingly enough when either GreaseMonkey or IETab is also installed the problem goes away. Both have components that call nsIContentPolicy FWIW.

Philip, this patch wasn't backed out that I know of. Can you possibly hunt down a regression range, or get someone to help with doing that?

(In reply to comment #34) > This went off my radar because it was marked fixed until a Flashblock user > complained again about Firefox 3.0RC1. Was this patch backed out or is it a > regression? > > Duplicated Flash content (when Flashbock in installed) seen using: Fwiw, I don't see any duplicated content with Camino 2.0a1 nightly trunk + FlashBlock and with Minefield RC1 Mac.

Sorry accidentally reopened. Re-closing.