152927 - 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

Status: VERIFIED FIXED

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

Description

[shrirang khanzode]

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

Comment 1

[rubydoo123]

and the embed or object has the parameter set to automatically start?

Comment 2

[shrirang khanzode]

yeah 'autostart =1' or 'true' is set in the source...checked it.

Comment 3

[rubydoo123]

assigning to peterl

Comment 4

[shrirang khanzode]

Attachment: 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 !

Comment 5

[Peter Lubczynski]

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!

Comment 6

[Peter Lubczynski]

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.

Comment 7

[Peter Lubczynski]

Attachment: possible fix? patch v.1

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.

Comment 8

[Peter Lubczynski]

Attachment: better patch v.2

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.

Comment 9

[Jaime Rodriguez, Jr.]

doug/serge: we are trying to wrap up 1.0.1. where are we on getting reviews for
the latest patch?

Comment 10

[Jaime Rodriguez, Jr.]

my apologies ... my last comment was meant for peterl. chalk it up to a copy and
paste error.

Comment 11

[Kevin McCluskey (gone)]

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. 

Comment 12

[Peter Lubczynski]

Attachment: new patch v.3

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)

Comment 13

[Kevin McCluskey (gone)]

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}
}

Comment 14

[Peter Lubczynski]

Attachment: patch v.4

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.

Comment 15

[Peter Lubczynski]

Attachment: patch v.5

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.

Comment 16

[Kevin McCluskey (gone)]

Comment on attachment 92301 [details] [diff] [review]
patch v.5

r=[e-mail]

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 92301 [details] [diff] [review]
patch v.5

sr=dveditz

Comment 18

[Jaime Rodriguez, Jr.]

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!

Comment 19

[chris hofmann]

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.

Comment 20

[Peter Lubczynski]

patch v.5 checked in on branch

Comment 21

[Jaime Rodriguez, Jr.]

shrir: pls verify this as fixed on the 1.0 branch. thanks!

Comment 22

[shrirang khanzode]

Jaime, there's no branch build out yet. Will verify this and check for any 
regressions as soon as the build comes out.

Comment 23

[shrirang khanzode]

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?

Comment 24

[Peter Lubczynski]

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.

Comment 25

[rpotts (gone)]

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

Comment 26

[Jaime Rodriguez, Jr.]

looks like the patch for bug #106253 was backed out of 1.0 in May. 

Comment 27

[Peter Lubczynski]

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

Comment 28

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to 1.1

Comment 29

[Peter Lubczynski]

patch in trunk, marking FIXED.

Comment 30

[shrirang khanzode]

verif on 1014 trunk , this works now.