XBL binding (e.g. Flashblock) leads to duplication of Flash objects

RESOLVED FIXED in mozilla1.9beta1

Status

()

defect
RESOLVED FIXED
13 years ago
9 years ago

People

(Reporter: philip.chee, Assigned: sicking)

Tracking

({regression})

Trunk
mozilla1.9beta1
All
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(4 attachments, 1 obsolete attachment)

Reporter

Description

13 years ago
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....
Flags: blocking1.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.
Flags: blocking1.9? → blocking1.9-

Comment 3

12 years ago
Posted file Minimized testcase
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.

Comment 4

12 years ago
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.

Updated

12 years ago
Keywords: regression
Summary: Flash plugin continues to display and run although the flash object has been removed from the DOM by Flashblock → XBL binding (e.g. Flashblock) leads to duplication of Flash objects
Reporter

Comment 5

12 years ago
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
Reporter

Comment 7

12 years ago
As the original reporter, I am renominating blocking-1.9 as there is now a minimized test case. Thanks Wladimir!
Flags: blocking1.9- → blocking1.9?
Assigning to me for now. Could someone find a regression range for this?
Assignee: nobody → jonas
Flags: blocking1.9? → blocking1.9+
Reporter

Comment 9

12 years ago
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
bug 315841 also sounds like a good candidate.

Biesi, could you take a look at this?
Assignee: jonas → cbiesinger
> 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?
Target Milestone: --- → mozilla1.9 M9
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.
Assignee: cbiesinger → jonas
Target Milestone: mozilla1.9 M9 → ---
Targeting M9 as this bug will block beta.
Target Milestone: --- → mozilla1.9 M9
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.
Reporter

Comment 20

12 years ago
> 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.
Posted patch Patch to fix (obsolete) — Splinter Review
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.
Attachment #285299 - Flags: superreview?(jst)
Attachment #285299 - Flags: review?(bzbarsky)
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?
Attachment #285299 - Flags: review?(bzbarsky) → review+
Attachment #285299 - Flags: superreview?(jst) → superreview+
Posted patch Patch v2Splinter Review
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.
Attachment #285299 - Attachment is obsolete: true
Attachment #286352 - Flags: superreview?
Attachment #286352 - Flags: review?
Attachment #286352 - Flags: superreview?(jst)
Attachment #286352 - Flags: superreview?
Attachment #286352 - Flags: review?(jst)
Attachment #286352 - Flags: review?
Attachment #286352 - Flags: superreview?(jst)
Attachment #286352 - Flags: superreview+
Attachment #286352 - Flags: review?(jst)
Attachment #286352 - Flags: review+
Comment on attachment 286352 [details] [diff] [review]
Patch v2

Actually, this is a beta blocker, so no approval needed.
Attachment #286352 - Flags: approvalM9?
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!
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
I wonder whether we can add a test for this...
Flags: in-testsuite?
Could we add a reftest somehow? Do reftests and plugins work together?
Reporter

Comment 29

12 years ago
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.
Status: RESOLVED → VERIFIED
Depends on: 401470
This has been verified via backout to be the cause of bug 401470.

Updated

12 years ago
Depends on: 401503
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.

Comment 32

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

Comment 33

12 years ago
(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 ?
Depends on: 401747

Updated

12 years ago
Depends on: 403360
Reporter

Comment 34

11 years ago
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.
Status: VERIFIED → REOPENED
Flags: blocking1.9.0.1?
Resolution: FIXED → ---
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.
Reporter

Comment 37

11 years ago
Sorry accidentally reopened. Re-closing.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago11 years ago
Resolution: --- → FIXED
Flags: blocking1.9.0.1?
You need to log in before you can comment on or make changes to this bug.