106253 - Plugins aren't adding themselves to load groups

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Judson Valeski]

This causes nsIWebProgressListener "document done" notifications to fire before
a plugin is done (which is wrong; affects progress bar among other things).

Here's the offending code. I'm hoping Rick has the best ideas on how to get the
"right" load group in there :-)

http://lxr.mozilla.org/seamonkey/source/modules/plugin/base/src/nsPluginHostImpl.cpp#5246

Comment 1

[rpotts (gone)]

my suggestion is to get the loadgroup from the document.

so, right after getting the document URL, i'd get the load group so...

something like this:

  doc->GetDocumentLoadGroup(getter_AddRefs(loadGroup));
and then pass 'loadgroup' into NS_OpenURI(...).

-- rick

Comment 2

[rpotts (gone)]

Attachment: untested patch...

Comment 3

[rpotts (gone)]

Attachment: here is a more complete patch...

Comment 4

[rpotts (gone)]

It is important to remember that this patch will defer the firing of the
javascript onLoad event until *after* the URL referenced in the plugin tag is
completely loaded.

if this URL is big (ie. a video stream) then the onLoad event will not fire for
a while :-)

I believe that this is the correct behavior... but we should verify that this
doesn't break backward compatability with NS4 and IE 5(-)
-- rick

Comment 5

[Judson Valeski]

av, who can best verify this?

Comment 6

[av (gone)]

Probably me or peterl. Can you advise on how to see the initial problem best?

Comment 7

[Peter Lubczynski]

*** Bug 92989 has been marked as a duplicate of this bug. ***

Comment 8

[Peter Lubczynski]

I tried this patch the other day as I thought it might fix bug 105780 which was
is a problem on http://www.the-xfiles.com where the status bar will continually
show the plugin loading.

Looks good, but there's one missing // XXX: Null LoadGroup? in
nsPluginStreamListenerPeer::SetUpCache(). I don't even recall what the purpose
of this method is or if it's just bit rotted away. Andrei, do you?

Comment 9

[Judson Valeski]

we're close here. av, what about SetUpCache()?

Comment 10

[Arun Ranganathan]

Since this was topembed+, I've nominated it nsbeta1+ because of interest in bug
106411 .

Comment 11

[Peter Lubczynski]

Rick, does this just need review?

Comment 12

[oliver jehle]

*** Bug 115998 has been marked as a duplicate of this bug. ***

Comment 13

[oliver jehle]

when will this patch included in the nightly build ????

Comment 14

[rpotts (gone)]

I guess my only question is whether this is the 'right thing to do' ??

In the case of a stream (or very large file) given to the plugin, the OnLoad
event will not fire until AFTER the plugin has consumed all of the data...

Is this the 4.x behavior?  If so, then the patch does the 'right thing'...

-- rick

Comment 15

[oliver jehle]

when i look at my java example and the handling with IExplorer (Sorry, i know,
it's not a reference but, but the platform intranets will be developed on :-(((
) then it's correct, that onInit will be fired when the applet is intialized....
there exist a lot of applications depend on onInit will be fired when the plugin
is initialized.

i don't know about shockwave and other plugins, but  when a onInit handler 
reference them and they arn't initialized, there are the same problems with
java... ( problems with javascript, that the method is not registered).

if a site has a plugin referenced on the page, they need it for working with the
site... 


there exists a lot of live-connect bugs.. this will solve some of them....

Comment 16

[Peter Lubczynski]

Attachment: same patch updated to the trunk tip

Comment 17

[Peter Lubczynski]

Attachment: tester plugin (npbasic.dll)

Comment 18

[Peter Lubczynski]

Attachment: testcase for plugin with large initial stream

This testcase uses the basic plugin in the previous attachment. It points to a
src that will take a long time to download (lxr of nsCSSFrameConstructor.cpp)
and fires an alert in the body's onload handler.

The good news is that it looks like 4.x has the same behavior of firing the
onLoad handler after the initial stream is done. I have not tested the other
ways to create streams.

However, I'm still getting the "Starting plugin..." message stuck in the status
bar. Is this supposed to fix that too?

Comment 19

[Peter Lubczynski]

Attachment: simple flash testcase

Comment 20

[oliver jehle]

Attachment: Testcase for Java-Javascript OnLoad problem

in this textfile you find 3 files, 2 html and 1 java... you get javascript
errors when fireing onLoad before intializing.

Comment 21

[Winnie Lam]

adding adt1 to status whiteboard as per discussion with beppe.

Comment 22

[Jaime Rodriguez, Jr.]

Changing to ADT3 per ADT triage. This sounds more like an annoyance, than any
real damage to the user. Can someone pls explain why this is important?

Comment 23

[oliver jehle]

because a lot of web sites with plugins need the proper handling of the 
onLoad tag with for example java.... 

perhabs not in the internet zone, but in the intranet zone you will find
a lot of examples, working with ie and java, but not with mozilla and java... 
because onLoad will not wait until java is intialized... 

so i think, its an important thing...

Comment 24

[Boris Schaeling]

I'm still not sure if this bug is related to 108601 (we experience 
java/javascript-onload-problems, too). However severity of "our" onload-problem 
is set to critical. Thus I agree with Oliver that this is not an annoyance but 
a bug which has to be fixed. 

Comment 25

[rpotts (gone)]

personally, i think this is an important bug to fix...  

i beleive that another benifit of this patch is to allow plugin requests to be
cancelled when a page transition occurs...  Currently (in Nav 6.21) if a plugin
is in the process of downloading a LARGE document, i believe the download
continues even *after* the page load has been cancelled :-(

-- rick

Comment 26

[Peter Lubczynski]

You've got my r=peterl on this approach. I think attachment 74092 [details] [diff] [review] does the right
thing. This patch does not appear to fix bug 106411.

Comment 27

[oliver jehle]

will this patch included to version 1 ??? target milestone is release 1... 

Comment 28

[Johnny Stenback (:jst)]

Comment on attachment 74092 [details] [diff] [review]
same patch updated to the trunk tip

sr=jst

Comment 29

[Judson Valeski]

Comment on attachment 74092 [details] [diff] [review]
same patch updated to the trunk tip

carrying over peter's r=, and approving for 1.0 checkin.

Comment 30

[Peter Lubczynski]

Since this fix delays the call to the onLoad handler until the plugin stream is
complete, is this going to effect the results of the page-load performance tests?

Comment 31

[scottputterman]

Adding adt1.0.0 nomination keyword.

Comment 32

[oliver jehle]

to comment 30...

>Since this fix delays the call to the onLoad handler until the plugin stream is
>complete, is this going to effect the results of the page-load performance tests?

perhabs it will have longer to give the control back to the user.. but for the
end-user, the correct display of the page is more important i think... 

Comment 33

[Jaime Rodriguez, Jr.]

Pls land this on the trunk to bake for a few days, have QA look, and make sure
there are no regressions introduced, then come back to us for approval - ADT

Comment 34

[Jaime Rodriguez, Jr.]

Removing adt1.0.0. Pls renominate once it has been on the trunk a couple days,
and been tested. 

Comment 35

[rpotts (gone)]

the patch is checked into the trunk.

Comment 36

[oliver jehle]

short question... 

why you don't add also java plugins to the load groups ???

Comment 37

[Paul Wyskoczka]

Shrir, can you test this fix and update the bug with your results.  Thx.

Comment 38

[shrirang khanzode]

bug 106411 hinders me from verifying this bug 'visually' if I understand this 
bug correctly. If there is any other way that I can check this, or someone can 
help in verifying this, I will appreciate it. Thanks !

Comment 39

[Peter Lubczynski]

Shrir, you can see this fix by having an event fired in your page's onLoad
handler.  Before this fix, onLoad would fire BEFORE the URL pointed to in the
EMBED's SRC attrribute (or OBJECT's DATA) was completely downloaded. Now, with
the fix, onLoad won't be fired until AFTER the plugin's "data" is done. We
should do like 4.x now.

Comment 40

[shrirang khanzode]

Attachment: embed and object testcases

Confirmed that this works now. onLoad now only fires only after the plugin has
loaded. verified fixed on trunk builds 0416. Attached two tescases work like
Peter mentioned.

Comment 41

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADTs behalf) approval for checkin to 1.0 branch. Pls check this
into the 1.0 branch and trunk today. After, this has landed on the branch, pls
add the fixed1.0.0 keyword.

Comment 42

[rpotts (gone)]

patch checked into the MOZILLA_1_0_0_BRANCH.

marking fixed.

Comment 43

[Chak Nanga]

Re-opening this bug and backing out the patch from Mozilla 1.0 branch.

This was causing a crash in RealPlugin when playing Real videos as described below:

1. Page has links to several Real videos and an embedded real plugin to display
the videos.

2. Click on a link to play a video - as the content begins to load, click on
another link to play a different video. 

4. Crash.

Requesting driver approval to back this out of mozilla 1.0 branch

Comment 44

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

a=dbaron to back out of 1.0 branch

Comment 45

[Chak Nanga]

Patch at http://bugzilla.mozilla.org/attachment.cgi?id=74092&action=view has
been backed out from the Mozilla 1.0 branch.

This bug is now reopened

Comment 46

[Peter Lubczynski]

Note: This was backed out of the 1.0 branch because of this Bugscape crasher:
http://bugscape.mcom.com/show_bug.cgi?id=14295

Now the trunk is different than the 1.0 branch w.r.t when the document's onLoad
handler fires with plugins on a page.

Comment 47

[shrirang khanzode]

removing 'fixed 1.0.0' keyword since this was backed out of the branch.

Comment 48

[Jaime Rodriguez, Jr.]

Removing adt1.0.0+, becasue this has been reopened, and backed-out of 1.0.

Comment 49

[Keyser Sose]

*** Bug 121641 has been marked as a duplicate of this bug. ***

Comment 50

[Peter Lubczynski]

-->bringing bug status into sync with reality, the patch is already on the
trunk. See the bugscape bug for the problem of Real crashing when the load group
request is canceled.

Comment 51

[shrirang khanzode]

v