85334 - Shockwave Web installs broken.

Status: VERIFIED FIXED

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

Description

[John Pfeiffer]

The installation process quits because the initial ping fails. It appears that 
Netscape 6.1b1 does not receive the entire buffer from the pinger while the 
browser is viewing the Shockwave completion page 
(http://www.shockwave.com/download/completion/index.html). This is the page that 
the Shockwave web installer launches the browser to in order to complete the 
rest of the Shockwave installation. Currently the pinger sends between 15 and 
16KB of data to the client during one of these initial pings (the result of a 
PostURLNotify issued by the player). On Netscape 4.7, this initial ping causes 
Netscape to cycle thru the WriteReady-Write loop many times before the entire 
buffer is received. Netscape 6.1b1 only cycles thru this WriteReady-Write loop a 
couple times, and thus does not receive the entire buffer. I have noticed that 
this bug does not occur on all pages. For instance, this bug does not occur 
while viewing this page: 
http://poppy.macromedia.com/~cmckeon/content/shockwavemovie.html
How to reproduce:
Since this bug is the result of a failed ping, one can reproduce this bug by 
forcing a re-registration.
1. With Shockwave player already installed, delete the following three registry 
entries:
HKCU\Software\Macromedia\Shockwave 8\nextpingdate
HKCU\Software\Macromedia\Shockwave 8\sw702down
HKCU\Software\Macromedia\Shockwave 8\sw702installed
2. Go to http://www.shockwave.com/download/completion/index.html
3. Ping fails, error message.

We are continuing to look at this problem to determine the cause of the failed 
ping.

Comment 1

[shrirang khanzode]

adding macromedia guys to cc list

Comment 2

[Arun Ranganathan]

shrir, does faking out the user-agent (by editing all-ns.js) make any difference?

Comment 3

[Peter Lubczynski]

To have Shockwave.com set a cookie and unblock Netscape 6.x, go to this URL:

http://www.shockwave.com/bin/shockwave/entry.jsp?skip=net6

Comment 4

[Arun Ranganathan]

More from Paul Betlem:

I don't think this is a user-agent issue (and actually has nothing to do
with Shockwave.com browser detection logic). The problem is recreated by
running the Shockwave Player web installer for Netscape, manually copying
the plug-in and class files from the Navigator plugins folder to the NS 6
plugins folder, and deleting the three registry entries shown below (which
tricks Shockwave into initiating the http post):

HKCU\Software\Macromedia\Shockwave 8\nextpingdate
HKCU\Software\Macromedia\Shockwave 8\sw702down
HKCU\Software\Macromedia\Shockwave 8\sw702installed

The final step is to navigate to any Shockwave movie, such as
http://www.macromedia.com/shockwave/welcome/

This initiates the post and demonstrates the problem.

Comment 5

[Arun Ranganathan]

cc'ing karnaze.
karnaze, this appears to be an rtm issue since it's shockwave's latest escalation.

Comment 6

[Peter Lubczynski]

Comments from John Pfeiffer:

Hi Peter,

Our WriteReady function restricts the number of bytes per Write that we
will accept from Netscape's stream buffer. What seems to be happening is
upon subsequent Writes, the pointer to Netscapes stream buffer is not
getting incremented. So, instead of getting a complete buffer of 16384
bytes, we're getting the first 8192 bytes twice.

Hope this helps,

John Pfeiffer

Comment 7

[Peter Lubczynski]

This should be easy to fix. First, a log from npspy would be helpful to confirm
the problem and compare with from 4.x.

The internal buffer in Netscape 6 is set to 16k which means that Necko (our
network library) will only send chunks of max 16k in size for the
WriteReady/Write loop. When we run out of that 16K, we free up the buffer if the
buffer wasn't created in NP_NewStream. 

We were getting the wrong data before because we were going down the code path
of RequestRead, but I thought I had fixed that. What build do you have (Gecko
XXXXX)? 

Andrei, here's the code in question:
http://lxr.mozilla.org/seamonkey/source/modules/plugin/nglsrc/ns4xPluginInstance.cpp#150

Comment 8

[Peter Lubczynski]

Andrei, this is still part of the 4.x buffer curruption. 

What happens is in ODA, we get a large amount of data, more than WriteReady
returns (and more than the plugin is willing to take >8k). All of this data is
stored in mStreamBuffer anyway for the duration of the ODA call.

See:
http://lxr.mozilla.org/seamonkey/source/modules/plugin/nglsrc/ns4xPluginInstance.cpp#201

So, I hacked it with a strncpy() to copy the data down and that seemed to get me
farther, but I later crash inside the plugin. Perhaps I should just move the
pointer? There could be more than one problem here.

Comment 9

[Peter Lubczynski]

Attachment: NP log of Nav 4.x

Comment 10

[Peter Lubczynski]

Attachment: *** USE THIS ONE **** (previous attachment is the wrong log)

Comment 11

[Peter Lubczynski]

Did I accidentally mark this confidential? Making public again so everyone can
read (nothing private in the bug).

I've been looking at the code in ns4xPluginStreamListener::ODA and it seems more
natural to have the the call to Read() only consume as much as WriteReady
returns.   Would it be okay to only consume as much data as the plugin needs in
each call to ODA? cc:ing DougT (for input on this and below)

I sort of had this working except I got stuck in a loop with the subsequent
WriteReady's. I thought I had gone down the wrong path, but then I read the old
4.x plugin API docs on WriteReady:
http://developer.netscape.com/docs/manuals/communicator/plugin/pgfn2.htm#1007417

>If the plug-in receives a value of zero, the data flow temporarily stops.
>Communicator checks to see if the plug-in can receive data again by resending
>the data at regular intervals.

Currently the code does this:
      // if WriteReady returned 0, the plugin is not ready to handle 
      // the data, return FAILURE for now
      if (numtowrite <= 0) {
        rv = NS_ERROR_FAILURE;
        goto error;
      }

No wonder we fail. This looks like problem #2 (first one seemed to be a
"currupted buffer" as explained above in the bug). Also, can someone explain
what are "regular intervals" as per spec? Maybe this is just a red hering and
I'm barking up the wrong tree.

But, looking at the old "Classic" codebase, looks a lot simpler:

http://lxr.mozilla.org/classic/source/modules/plugin/nglsrc/ns4xPluginStream.cpp

Comment 12

[Arun Ranganathan]

putting 'pdt+' in the status whiteboard.  PDT has requested that this be 0.9.2
material -- pls. re-set target milestone whenever possible.

Comment 13

[Peter Lubczynski]

marking mozilla0.9.2 per meeting. I think this is the last bug blocking
Shockwave.com. This is a bad streaming bug.

Comment 14

[Asa Dotzler [:asa]]

bugzilla is mozilla's bug tracking database. the only bugs that should be in the
netscape confidential group are browser exploit/security bugs.  please remove
the confidential flag or mark this bug invalid. Thanks.

Comment 15

[Arun Ranganathan]

Asa, 
This has been 'untagged' confidential.  Is it still showing up as confidential
on your radar?

Comment 16

[Peter Lubczynski]

Opened [Bug 86473] nsIStreamListener::ODA needs to be able to consume less than 
|count| for dealing with "problem #2". 

Otherwise, I don't see a way to fix the problem of Shockwave returning zero from 
 WriteReady. We must get back to the event loop so that Shockwave can be 
given the chance to process the data it was given. We can't do this if we are 
forced to consume all data in each call to ODA without caching in yet another 
buffer. But we can't cache this way, because for a large stream like radio, it's 
possible that we have a very fast network (LAN). This would result in a HUGE 
buffer, one that the plugin could never "catch up" to. 

Comment 17

[Arun Ranganathan]

Marking this rtm stopper -- it's already 0.9.2 --  since av believes that bug
58128 has symptoms that this will alleviate.

Comment 18

[av (gone)]

So, this bug is blocked by bug 86473. We need to consolidate both bug status' 
somehow, either to postpone this one or to pump up the priority for 86473.

Arun?

Comment 19

[selmer (gone)]

Please update the status whiteboard with your eta.

Comment 20

[Peter Lubczynski]

Andrei is on vacation till Tuesday....taking over.

With a patch in bug 86473, now I can "pause" the channel. I'll begin to re-write
ODA but I probably won't finish before the weekend and I'd like Andrei to verify
it anyway as it would be a high risk change.

It would greatly speed thing up if I could fine more testcases in which
Shockwave consumes less data than we try to give it (WriteReady returns zero)
besides just registration.

This was working at one time, so I think we are close once this final bug if
fixed. I am a little worried if the "Web Install" uses any of these other
features of the 4.x API which have open bugs. I know
navigator.plugins.refresh(true) was having problems but only with XPCOM plugins,
for example. 

Comment 21

[Peter Lubczynski]

--->peterlubczynski

Comment 22

[selmer (gone)]

we'd like to branch no later than Monday.  Is there anything that might get this
resolved by 6/25?

Comment 23

[Peter Lubczynski]

Attachment: patch for first stage of work

Comment 24

[Peter Lubczynski]

Here's the first patch to get going. Try it out, if you can. It seem to kinda
work well for me (registration completes and I get the "welcome" screen").
However, that's in the debugger so it's probably slowing things down enough that
we never get a "zero" write ready. I'll keep trying.

The second part isn't so easy. It entails using the "pause/resume" being
implemented in bug 84673. My plan is to add an nsITimer and ONLY have it start
to fire when WriteReady returns 0. At the same time, I'll pause the channel and
try to consume any left over data. When the timer fires, I'll call ODA again to
try to process the data. If returns NS_OK that means we can Resume the channel.

The nsITimer should only fire once with WriteReady governing when it should
fire. Actually, it would be better just to have some kind of PL callback I can
post and have my Notify() called without messing with timers. Anyone know of an
example? 

Comment 25

[Peter Lubczynski]

Well, I've got the framework up, but I don't understand Shockwave. It never
returns anything but Zero for NP_Write but NP_WriteReady returns 8k. The timer
seems to be working as I get a dozen or so callbacks when I crack up the
timeslice to something noticable.

Why does shockwave always return zero?

Comment 26

[chris hofmann]

the 6/27 eta sounds ok with the info I know.

Comment 27

[Peter Lubczynski]

Good news! I almost have this working and it won't be quite as risky as I had
thought. Updating ETA to Monday, I still need to do some testing. It's actually
a combination of problems:

1) Streaming is currupted. Fixed that with a |strncpy| of the buffer in the
loop. Not very efficient, but lower risk (and my other idea) and no more
Shockwave errors! FIXED (pending heavy testing)

2) As Peter G mentioned, the Shockwave registration DLL changes the plugin entry
point table after registration. Turns out, he was right, we were passing in a
pointer on the stack so when they flipped the switch after registration,
everything looked broken because we were using the old function table. Easy, low
risk fix to simply use the member ns4xPlugin::fCallbacks instead of a stack
pointer. FIXED.

3) Finnally, the last problem I have is with loading the movie. It seems that I
have to click the reload button to "kickstart" it. It sometimes loads. I'm
trying to figure out why now.

Comment 28

[Peter Lubczynski]

New depends on bug 87397. That seems to be what's blocking the new instance from
working.

Comment 29

[Peter Lubczynski]

Attachment: PATCH TO FIX STREAMING PROBLEMS #1

Comment 30

[Peter Lubczynski]

That last patch will get us a litte further, but I'm still having problems with
the final display what I think is bug 87397.

I'm seeking reviews for this patch, here's what I've done:

1) Rearranged how GetEntryPoints is called on PC platforms so that a ns4xPlugin
member variable is used instead of a stack pointer.

2) After fixing that, I was still getting currupted streams and errors as a
result. I had to fix up enough of ODA to at least by. It seems our position in
the stream was incorrectly being set. I fixed this so that the position is set
to what the plugin actually returns it consumed. Isn't this the correct thing to
do? THEN, since this is in a WriteReay/Write loop, the plugin expects the new
data on the next loop to already be shiffted to the behing of the buffer. This
part needs testing as it's a high-risk change to the core of 4.x plugin streams
(and effects almost all  4.x plugins, XPCOM is not effected). 

Comment 31

[Peter Lubczynski]

Doug, Darin, or Brian, can you [s|r] the patch in this bug too, #39895? Thanks!

Comment 32

[Brian Nesse (gone)]

One evil tab in the comment "create the new plugin handler" otherwise seems
solid. r=bnesse.

Comment 33

[Marc Attinasi]

Peter, 6/25 was yesterday - please update the ETA and add any pertinent info - 
PDT wants this resolved - thanks!

Comment 34

[Peter Lubczynski]

This bug was dependant on fixing bug 87397. That patch is already in the trunk
baking, with this one close behind it. All I need is a super review for this patch.

Comment 35

[Peter Lubczynski]

Attachment: more complete patch, addressing some lingering issues

Comment 36

[Peter Lubczynski]

Okay, that last patch gets us to the home stretch, but it isn't going to win the
ball game. Alas, one more problem discovered:

If you notice in the patch, I've added a check for a URI vs. URL and return
error.   If that's not there, NewStream/DestroyStream are called on the
javascript:window.location URL and Shockwave crashes! However, the check is in
the wrong place, it's too deep and too late. A stream gets created anyway and
that's the very problem I was trying to prevent. 

I'll fix up the patch in the morning, but after messing around with it for a few
hours, I think the safest thing to do is do the URL vs. URI check in OSR to
prevent any calls into the plugin.

Oh...and I've added one more thing to that last patch, a fix to the object frame
to destroy the previous document (and stop all associated plugins). Without this
I was getting other crashes.

Comment 37

[Peter Lubczynski]

cc:ing mkaply for any OS/2 impact on the way we call GetEntryPoints().

The following patch, as scarry as it looks, makes Shockwave install and
registration work till the end! I'm seeking all new reviews. To recap the issues:

1) Rearranged how GetEntryPoints is called on PC platforms so that a ns4xPlugin
member variable is used instead of a stack pointer.

2) After fixing that, I was still getting currupted streams and errors as a
result. I had to fix up enough of ODA to at least by. It seems our position in
the stream was incorrectly being set. I fixed this so that the position is set
to what the plugin actually returns it consumed. Isn't this the correct thing to
do? THEN, since this is in a WriteReay/Write loop, the plugin expects the new
data on the next loop to already be shiffted to the behing of the buffer. This
part needs testing as it's a high-risk change to the core of 4.x plugin streams
(and effects almost all  4.x plugins, XPCOM is not effected). 

3) Added checks for a URI vs. URL. The reason is because we may have a
Javascript handler and we don't want to do a NewStream/DestoryStream. This
should prevent any stray calls into the plugin. Doug/Darin, am I doing this
correctly?

4) In nsObjectFrame, I'm now clearing out the previous content viewer. I got
this idea from Hyatt so cc:ing for comments. Basically, because of the special
way registration happens, the plugin instances on the previous page needs to be
stopped so the new one can take over. By setting the previous content viewer to
null, it forces the document (frames and plugins) to get destroyed right away.
Should also fix any lingering reload issues.

5) Finnally, I've added a special HACK for Macromedia's "pinger". For some
reason, if we call URLNotify for the pinger URL, the plugin crashes! It's a hack
and it's ugly, but after speaking with Peter G from Macromedia, I don't see any
better way of fixing this. And..it's very low risk.

Comment 38

[Peter Lubczynski]

Attachment: PATCH TO ACTUALLY MAKE THIS WORK (please review)

Comment 39

[av (gone)]

Looks good to me in almost all aspects. But I still have a couple of 
questions. How does this patch handle the situation when not entire stream is 
consumed? Are you planning to introduce a timer? Another thing, just wondering, 
when you do URI vs. URL test, why nsCOMPrt<nsIStandardURL> rather than 
nsCOMPtr<nsIURL>? I am asking because I did the same thing in one of my latest 
fixes and used the latter, maybe it needs to be fixed.

Comment 40

[Peter Lubczynski]

Good questions.

I've put in an ASSERT to see how often that happens and I could not get it to
fire on registration. I'm planning to open another bug on that issue, but at
this time I don't believe it will be a rtm stopper. I probably will use a timer.


I, too was using nsCOMPtr<nsIURL> checks until I found that the call dropped
through!!! nsCOMPrt<nsIStandardURL> seemed to work a lot better. Does anyone
know what the difference is?

Comment 41

[Peter Lubczynski]

Attachment: updated patch, cleaned up and does not include fix #3, the URI vs. URL check

Comment 42

[Peter Lubczynski]

After an e-mail exchange with PeterG about if GetURL and PostURL should cause a
NewStream, I removed all URI vs. URL tests and registration still worked just
fine. That appears to be a non-issue so I removed it from the patch.
NewStreams()'s error code decides if the plugin takes the stream or not, which
is probably why it's worked so far.

Andrei, I think that answers all your questions. Can I get an r=?

Comment 43

[Mike Kaply [:mkaply]]

Can you make this XP_WIN for now so I can verify that it builds/works 
on OS/2?

Thanks

Comment 44

[Peter Lubczynski]

Attachment: revised patch adding XP_WIN and XP_MAC

Comment 45

[av (gone)]

r=av

Just minor things which you may or may not take into account. To me the 
following two lines look contradicting to each other. 

+#if defined(XP_PC) && !defined(XP_WIN)
+    // XXX this only applies on Windows

Also, from my experience drivers don't like when commented out code is left in 
the repository.

Comment 46

[Brendan Eich [:brendan]]

IIRC, XP_PC is defined for x86 machines, even if the OS you run on 'em is OS2. 
This area is messy; cc'ing cls for guidance.

/be

Comment 47

[Peter Lubczynski]

Attachment: updated patch

Comment 48

[Peter Lubczynski]

I put the XP_PC code back the way it was and special cased XP_WIN as was done
below for XP_MAC. That should prevent me from accidentally breaking something on
another platform.

Comment 49

[Marc Attinasi]

Looks good. Shouldn't '#elif XP_WIN' actually be '#elif defined(XP_WIN)' for
consistency? Also, what have you done to test this patch so far? Assuming you
have tested it thoroughly (for other plugins too?), sr=attinasi

Comment 50

[Peter Lubczynski]

Along with bug 87397, I've tested this on Windows with all the top plugins and
tested it on Mac. Shockwave.com works nicely. Just to be safe, I'm checking into
the trunk for baking.

Comment 51

[cls]

XP_PC should really just go away (or be replaced by XP_DOS or something).  Right
now XP_PC only refers to win32 & OS/2 not all x86 boxes (that's what _X86_ is
for).  The current patch (attach 40502) looks fine.

Comment 52

[Peter Lubczynski]

Marc's change made, patch checked into the trunk for baking. Pending approvals
and all goes well tomrrow, I'd like check this into the branch on Friday evening.

Srirang, can you give this some testing?

Comment 53

[Asa Dotzler [:asa]]

pushing out. 0.9.2 is done. (querying for this string will get you the list of
the 0.9.2 bugs I moved to 0.9.3)

Comment 54

[Brian Nesse (gone)]

Peter, you've caused a crasher with your patch. You removed code from the mac 
build that was initializing the callback structure with 0's.
-    memset((void*) &callbacks, 0, sizeof(callbacks));

This is now only happening in the #ifdef XP_WIN code in the ns4xPlugin 
constructor.

+  memset((void*) &fCallbacks, 0, sizeof(fCallbacks));

In a plugin which doesn't support certain callbacks (such as GetValue in this 
case), the code in ns4xPluginInstance::GetValue

if (fCallbacks->getvalue) {
    CallNPP_GetValueProc(fCallbacks->getvalue, ...
}

fails in an ugly way ;)

You should make sure the callback struct is initalized to 0 on all platforms...

Comment 55

[Peter Lubczynski]

Attachment: simple patch to do memset of fCallbacks on all plaforms

Comment 56

[Brian Nesse (gone)]

r=bnesse.

Comment 57

[Marc Attinasi]

sr=attinasi on the patch to clear the callbacks.

Comment 58

[Mark Hammond [:markh] [:mhammond]]

The fix to this bug has caused a "graceful crash" with xpcom based plugins - 
such as the standard plugin sample in mozilla/modules/plugin/test.

As an xpcom plugin is initialized we get an assertion failure.  As the plugin 
is terminated, we de-reference a NULL pointer - but this dereference is wrapped 
in a win32 exception handler - causing us to get one of the cute "this plugin 
has caused an error" messages, and an assertion.

To demonstrate the problem, simply build the sample plugin 
(mozilla/modules/plugin/test) then load one of the test HTML files from the 
same directory.

To confuse the issue, the sample plug-in has a trivial makefile bug caused by a 
bad checkin - see bug 88789.  You may need to fix the makefile before making 
the sample - first character of the first line currently is a stray '|' 
character!

I have a trivial patch that removes the obvious error messages.  However, I 
don't believe it is the correct fix - we are clearly inside the "4.1 compatible 
plugin loader" code, but we are a new-style XPCOM plugin.  Indeed, we seem to 
be the canonical example of the XPCOM plugin.  There is code that supposedly 
handles new style plugins, but gets as far as failing a QI for nsIPlugin on the 
module factory.  This was probably broken ages ago when the sample plugin was 
upgraded to the new NS_IMPL_NSGETMODULE module loader scheme.

I haven't attached the patch - I would much prefer someone more knowledgable 
with this code to repro the error, and see if there is an obvious reason why we 
are in the failing code in the first place.

Comment 59

[Peter Lubczynski]

Okay, I see the problem. We try to create a 4.x style plugin for an XPCOM plugin
as a last resort. The problem here is that failure isn't set up correctly
because the calling function is checking for a null aLibrary returned. Attaching
a patch to fix this.

Comment 60

[Peter Lubczynski]

Attachment: patch rearranging logic to prevent crash

Comment 61

[Peter Lubczynski]

Attachment: patch to fix crash AND also fix OS X plugins

Comment 62

[av (gone)]

r=av

Comment 63

[Marc Attinasi]

sr=attinasi

Comment 64

[Peter Lubczynski]

Ahh, so the plot thickens. As that last patch fixes registration issues, later
down the line we crash while trying to play TankWars when downloading 3dGroove.
The problem is because of a reframe being caused indirectly from a WM_FONTCHANGE
message. Basically, bug 85422 now blocks me. 

Good news though is that to that bug, I've attached this patch:
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=40924

Which seems to fix the crash by not doing a reframe. Over to that bug to lobby.

Also, Shrir, this looks to be Windows only, can test Mac? Thanks!

Comment 65

[Peter Lubczynski]

Attachment: one more patch, revert changes for CARBON

Comment 66

[Peter Lubczynski]

Patches in trunk. "Registration" should now be working and other plugins should
still work.

Comment 67

[Marek Z. Jeziorek]

Peter, this is PDT+ bug. Tomorrow, on Tuesday, we'll try to build the first RTM
candidate. It would be good, if this could be resolved ASAP.

Comment 68

[Peter Lubczynski]

Checked into the banch last night, marking FIXED.

Comment 69

[shrirang khanzode]

this is working on today's win 0705 trunk and branch. Registration;installation 
completes fine.

However,having a problem on mac...Will check on another mac and comment again.

Comment 70

[shrirang khanzode]

mac just freezes..

Comment 71

[Peter Lubczynski]

Shrirang, can you be more specific? Were you testing the trunk or the branch?
Also, this bug is marked Windows NT, perhaps a new bug should be opened on this
issue.

Have you noticed any weirdness in general with Mac plugins in today's build
(trunk or branch)? Seems to me like suddenly we have a lot of Mac plugin bugs,
but I don't see a relationship.

Comment 72

[shrirang khanzode]

filed bug 89529 for the problem on mac. Pls see that bug for details. Cannot 
test the shockwave web install on mac build until that bug is resolved. 

Comment 73

[shrirang khanzode]

verified fixed on mac trunk and branch builds 070904/070903 resp. Had already 
verified on windows. Shockwave web install/update completes fine with no 
problem.