Closed Bug 152927 Opened 19 years ago Closed 19 years ago

can't script any plugin in nested <embed> tag inside an <object> tag from onLoad handler -- CNET radio does not play with Real because SetSource is called from onLoad

Categories

(Core :: Plug-ins, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.2beta

People

(Reporter: shrir, Assigned: peterl-bugs)

References

()

Details

(Keywords: testcase, top100, topembed, Whiteboard: [ADT1 RTM] [PL2:P1] [07/24])

Attachments

(2 files, 4 obsolete files)

I have seen this in a few bugs, logging a new one for clarity sakes.

seen on 0618 brnch NT, make sure you have realplayer installed.

steps:

go to the above page
Click on "Listen Live" under 'On Air Now'

'Change player' to realplayer inside the new window that pops up..

observe that the audio feed jsut does not start
and the embed or object has the parameter set to automatically start?
yeah 'autostart =1' or 'true' is set in the source...checked it.
assigning to peterl
Assignee: beppe → peterl
Priority: -- → P2
Whiteboard: [PL2:NA]
Target Milestone: --- → mozilla1.0.2
Attached file testcase
testcase shows that calling the realplayer scripting function from within the
document's onLoad() does not work, calling it thru submit works. Thx, Peter !
This is a serious problem -- calling any scripting function on a plugin during
the onLoad handler will fail. The reason for this is because plugins are tied to
the frame model rather than content/DOM. We do not create the plugin until we
reflow the layout frame. The onLoad handler fires before we have a chance to
create the plugin instance in the object frame. One solution to this problem is
outlined in bug 90268 where we would move plugin creation to content (like jst
recently did with iframes). But fixing that is not going to be easy.

I wonder if there is hack we can come up with that can be done in the meantime
to get C|Net radio working?

btw, thanks for testcase Shrir!
Depends on: 90268
Keywords: 4xp, testcase, top100
OS: Windows NT → All
Hardware: PC → All
Summary: realaudio feed does not start...sometimes → can't script any plugin from onLoad handler -- CNET radio does not play with Real because SetSource is called from onLoad
Whiteboard: [PL2:NA] → [PL2:P1]
Target Milestone: mozilla1.0.2 → mozilla1.2beta
Blocks: 143047
Keywords: nsbeta1
Whiteboard: [PL2:P1] → [ADT2 RTM] [PL2:P1] [ETA Needed]
Keywords: nsbeta1nsbeta1+
Update: This only happens in a "nested" tag situation. Removing the outer OBJECT
tag allows for scripting from onLoad.

For debugging, I'm setting a breakpoint in |nsDocShell::EndPageLoad| which calls
the onLoad handler.

Here is the root problem as to why it only fails in a nesting situation:
During Reflow, we call |CantRenderReplaceElement| on the outer ActiveX OBJECT
tag. This is an asynchronous function (via PLEvent) that causes the
CSSFrameConstructor to make frames for the contents of the non-renderable outer
OBJECT tag. It appears in |PresShell::ProcessReflowCommands| we call
|DoneRemovingReflowCommands| which causes the onLoad handler to fire while we
still have a CantRenderReplaceElement event in the queue.

I'm going to see if I can delay the onLoad handler if there is a pending
CantRenderReplaceElement.
Summary: can't script any plugin from onLoad handler -- CNET radio does not play with Real because SetSource is called from onLoad → can't script any plugin in nested <embed> tag inside an <object> tag from onLoad handler -- CNET radio does not play with Real because SetSource is called from onLoad
Keywords: patch
Whiteboard: [ADT2 RTM] [PL2:P1] [ETA Needed] → [ADT2 RTM] [PL2:P1] [7/17]
Attached patch possible fix? patch v.1 (obsolete) — Splinter Review
Here's a hack that seems to get C|Net Radio playing. It delays the calling of
the onLoad handler by means of adding these "dummy" requests in the pres shell.
When we call |CantRenderReplaceElement|, the patch adds two requests to the
load group. I still need to run more tests to ensure it doesn't break anything
other combinations of content.
Severity: major → normal
Priority: P2 → P1
Attached patch better patch v.2 (obsolete) — Splinter Review
Here's a patch I feel better about. Instead of re-using the pres shell's dummy
layout request, I give each CantRenderReplaceElement it's own. Using the
constructor/destructor of the PLEvent is a much better way to ensure we only
add and remove once from the load group per event which was a problem with the
last patch. So basically with this patch while we have a pending
CantRenderReplaceElement PLEvent, we'll have a dummy request in the load group
to delay the onLoad handler.
Attachment #91181 - Attachment is obsolete: true
Whiteboard: [ADT2 RTM] [PL2:P1] [7/17] → [ADT2 RTM] [PL2:P1] [07/22]
Whiteboard: [ADT2 RTM] [PL2:P1] [07/22] → [ADT1 RTM] [PL2:P1] [07/22]
doug/serge: we are trying to wrap up 1.0.1. where are we on getting reviews for
the latest patch?
my apologies ... my last comment was meant for peterl. chalk it up to a copy and
paste error.
I reviewed attachment(id=91806) but I suggested a different approach. The
current patch creates a dummy request instance for every replaced element. This
could be expensive on pages with lots of input elements. I suggested an
alternative approach of keeping a count of the number of replaced elements that
have not been loaded. The dummy request would not be removed until all of the
replaced elements had loaded. 
Attached patch new patch v.3 (obsolete) — Splinter Review
New in this patch:
 * keeps a count instead of allocating memory as suggested
 * ensures we've attempted to remove the request
 * isolated to object frame (applet/object/embed tags)
Attachment #91806 - Attachment is obsolete: true
I think  it would be a little easier to undertand and cleaner if the counter was
expanded to indicate whether the document has completed loading regardless of
whether it was waiting for the normal document to load (i.e no more reflows
pending) or replaced elements were still in the process of loading.

I think this could be accomplished by doing the following:

Change method name and signature of
HoldLoadGroupRequestCount(nsIPresShell::eHold_Increase)

 to something like the following:

NotifyLoadCompleted(PRBool aLoadIsComplete)

if (aLoadIsComplete) than decrement the counter
if (!aLoadIsComplete) than increment the counter

Change mHoldLoadGroupRequestCount to mLoadingCount

Get rid of having a separate flag for mAttemptedToRemove. Instead the
NotifyLoadCompleted(PR_FALSE) would be called from AddDummyLayoutRequest. This
would set the mLoadingCount to 1. Modify DoneRemovingReflowCommands to call
NotifyLoadCompleted(PR_TRUE). This would set the counter back to 0 and trigger a
call to Remove the DummyLoadRequest .


Some code snippets:


 FrameManager::DestroyPLEvent(CantRenderReplacedElementEvent* aEvent)
 {
  ...
  if (shell)
    shell->NotifyLoadCompleted(PR_TRUE);

   delete aEvent;
 }

CantRenderReplacedElementEvent::CantRenderReplacedElementEvent(FrameManager*
aFrameManager,
                                                              nsIFrame*     aFrame,
                                                              nsIPresShell*
aPresShell)
 {
   ...
  if (nsLayoutAtoms::objectFrame == frameType) {
    mPresShell = getter_AddRefs(NS_GetWeakReference(aPresShell));
    aPresShell->NotifyLoadCompleted(PR_FALSE);
  }


NS_IMETHODIMP PresShell::NotifyLoadCompleted(PRBool aLoadComplete)
{
  if (aLoadComplete) {
    mLoadingCount--;
    if (mLoadingCount <= 0) {
      RemoveDummyLayoutRequest();  
    }
  } else {
    mLoadingCount++;
  }
  return NS_OK
}


PresShell::AddDummyLayoutRequest(void)
{
    ...
      rv = loadGroup->AddRequest(mDummyLayoutRequest, nsnull);
      NotifyLoadCompleted(PR_FALSE);
     ...
}


void
PresShell::DoneRemovingReflowCommands()
{
  if (mRCCreatedDuringLoad == 0 && mDummyLayoutRequest && !mIsReflowing) {
   NotifyLoadCompleted(PR_TRUE);
    // RemoveDummyLayoutRequest();   <== This will be called from
NotifyLoadCompleted when the count drops to 0}
}
Whiteboard: [ADT1 RTM] [PL2:P1] [07/22] → [ADT1 RTM] [PL2:P1] [07/24]
Attached patch patch v.4 (obsolete) — Splinter Review
update patch to Kevin comments. However, I had a problem in that
|FrameManager::ReplaceFrame| eventually caused |DoneRemovingReflowCommands| to
be called and the onLoad would fire before the new frame was created. So, I use
a second lock in |nsCSSFrameConstructor:CantRenderReplacedElement| which did
the trick.

It seems that this code is called quite frequently, on at least every page. It
would be good to do some more testing, especially if something were attached to
the onLoad handler like in a cycle-through test.
Attachment #92132 - Attachment is obsolete: true
Attached patch patch v.5Splinter Review
After speaking with Kevin, patch v.2 with a few modifications would seem safest
for the branch while a better solution using the approach in patch v.4 could be
explored for the trunk.

Here's basically the same patch as v.2 except we will only add a request to the
load group if we are doing a CantRenderReplacedElementEvent on an objectFrame.
The request is added in the constructor and removed in the destructor of the
event.
Attachment #92258 - Attachment is obsolete: true
Keywords: adt1.0.1
Comment on attachment 92301 [details] [diff] [review]
patch v.5

sr=dveditz
Attachment #92301 - Flags: superreview+
adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch, pending
drivers' approval. pls check this in asap, then replace "mozilla1.0.1" with
"fixed1.0.1". thanks!
Keywords: adt1.0.1adt1.0.1+, approval
Comment on attachment 92301 [details] [diff] [review]
patch v.5

a=chofmann for 1.0.1.  add the fixed1.0.1 keyword after checking into the
branch.
Attachment #92301 - Flags: approval+
patch v.5 checked in on branch
No longer depends on: 90268
shrir: pls verify this as fixed on the 1.0 branch. thanks!
Jaime, there's no branch build out yet. Will verify this and check for any 
regressions as soon as the build comes out.
works like a charm, verified on 0723 branch . Also tested for regressions, none 
seen. Verified ! Is this not on the trunk yet ? what to do with the bug 
resolution?
I'm planning to land on trunk after getting drivers approval and then open a new
bug on expanding this fix to all types of frames.
it seems like we should check in the patch for bug #106253 too.  It's just a
different facet of the same problem -- that plugins aren't playing well with
loadgroups...

-- rick
looks like the patch for bug #106253 was backed out of 1.0 in May. 
C|Net defaults the SRC attribute to "" so I don't think the patch from bug
106253 would not have any effect here. The patch in bug 106253 was backed out of
the branch because it caused problems with Real described by Rick in this comment:
http://bugscape.mcom.com/show_bug.cgi?id=14295#c49
a=asa (on behalf of drivers) for checkin to 1.1
patch in trunk, marking FIXED.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
verif on 1014 trunk , this works now.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.