Closed Bug 108966 Opened 23 years ago Closed 23 years ago

shockwave player installation does not complete

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.7

People

(Reporter: shrir, Assigned: peterlubczynski-bugs)

References

()

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

seen on 1107 trunk build on win98, win2k

steps:
completely remove shockwave form your machine (registry, plugins foldeR)
go to the above url
download flash player (it installs and works fine)
Then relaunch browser and go to 'Games' and 'Arcade' and select any game
A message asking you to download shockwave player comes up, click on the button 
to download shockwave player file to desktop.

Launch the shockwave installer...see the installation proceed (browsers get 
detected) and then 6.x is relaunched and the welcome page appears.
However, instead of the registration window, I get a 'plugin not loaded, pls 
download' message.(instead of the registration window)

When I restart the browser and go to a game again, once in a while, I do get the 
registration window but cannot proceed beyond entering my name email 
address...clicking on the NEXT button just hangs the window.

something is wrong for sure. will check mac and see if it happens there too.
same on mac. os: all
OS: Windows NT → All
Hardware: PC → All
something regressed, do you know when?
Keywords: regression
seems like after 6.2 got released..wil try to find out the date when this 
happened...
This is likely caused by the same problem as in bug 113543. Looks like it's a
memory problem due to my addition of NewByteInputStream. I'll discuss in bug 113543.
Status: NEW → ASSIGNED
Depends on: 113543
Priority: -- → P1
Target Milestone: --- → mozilla0.9.7
No longer depends on: 113543
Second thought, this looks different. This is a regression from bug 105417. I've
already looks in the debugger, I know what's wrong:

witness NS_NewPostDataStream makes a copy of the post
buffer:http://lxr.mozilla.org/mozilla/source/netwerk/base/public/nsNetUtil.h#253

To do binary streams, I replaced this call with NS_NewByteInputStream. Witness
this does NOT copy the buffer:
http://lxr.mozilla.org/mozilla/source/xpcom/io/nsStringStream.cpp#655

Well, this sucks because the memory for the buffer is freed synchronously by the
plugin after the call so we post garbage.

Possible solution:
In the cases we call NS_NewByteInputStream, copy the buffer ourselves. The
problem is, when and where to free it? I imagine since NS_NewByteInputStream
uses a const buffer that it doesn't free it. Anyone know what combination of
stream and string class to automatically copy my 1-byte array of certain length
and free it when the stream is done? NS_NewPostDataStream did this correctly
with CStrings, but I may have nulls in my string. :(
sounds like nsIStringInputStream may help (see nsIStringStream.idl)

nsIStringInputStream::adoptData(char *data, int length)
nsIStringInputStream::shareData(const char *data, int length)

neither of these functions performs a memcpy. |adoptData| sets the data for the
stream, and the data will be |nsMemory::Free|'d by the stream when it is
destroyed.  |shareData| sets the data for the stream but does not free the data
when it is destroyed.

look in nsStringStream.h for the CID/ContractID of an implementation of
nsIStringInputStream.
i should have added that both of these functions will work with data that
contains embedded null characters or is not null terminated.  that is, unless
you specify a length of -1 in which case the impl will use strlen to determine
the data length.
Cool! I think that'll work. I'll try it first thing tomorrow.
Attached patch patch (obsolete) — Splinter Review
This seems to fix the Shockwave registration. I still need to a little more
testing...
Blocks: 113543
Keywords: patch, review
Attached patch updated patch (obsolete) — Splinter Review
Here's a slightly better patch that takes care of another bug - in the code I
put in, we didn't handle the case of a correctly passed in file path without
file://. Now if we can't convert the path, we just fallback to the raw data.

Tested with plugin testcase from Rob Hatfield.

I think that should be it. Please thoroughly review.
Attachment #60601 - Attachment is obsolete: true
Please interact with Peter about testing this with a test driver.
Attached file post_tester.zip
post_tester.zip

Here is an xp tester plugin I created for testing NPN_PostURL. The zip contains
a built debug plugin, a test html page with limited instructions on use, and
the diffs of what I did to the sample in order to re-build it. This plugin
works by specifying attributes (or parameters) on the tag which are then
directly passed to the NPAPI. QA can use this to verify this bug and others and
maybe even come up with variations to find other problems.

Andrei, can you review the patch?
Comment on attachment 60648 [details] [diff] [review]
updated patch

Looks good to me. r=av
Attachment #60648 - Flags: review+
Darin or Scott, can I get your super review? Thanks.
Keywords: edt0.9.4
Comment on attachment 60648 [details] [diff] [review]
updated patch

>Index: layout/html/base/src/nsObjectFrame.cpp

>+  const char * dataToPost = (const char *)aPostData;
>+  nsXPIDLCString filename;

these should be declared within

    if (aPostData) {

instead of outside this block.


>+      // convert file:///c:/ to c: if needed
>+      nsCOMPtr<nsILocalFile> aFile = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID);
>+      if (NS_SUCCEEDED(aFile->SetURL(dataToPost)))
>+        if (NS_SUCCEEDED(aFile->GetPath(getter_Copies(filename))))
>+          dataToPost = filename;

nit: how about replacing the second |if| statement with &&


>Index: modules/plugin/base/src/nsPluginViewer.cpp

>+  const char * dataToPost = (const char *)aPostData;
>+  nsXPIDLCString filename;

same thing here....

otherwise, i think this patch looks fine... cleanup, and sr=darin
Attachment #60648 - Flags: needs-work+
does this happen when you insert the following step in the steps to repro in the
first comment:

"shut down the client" then "completely remove shockwave..." Does this happen in
clients that don't allow auto update of plugins?
Attached patch corrected patchSplinter Review
Attachment #60648 - Attachment is obsolete: true
Attachment #61321 - Flags: superreview+
Attachment #61321 - Flags: review+
Patch in trunk, marking FIXED. I'll renomiate after some baking....
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Keywords: edt0.9.4, review
Resolution: --- → FIXED
*** Bug 113543 has been marked as a duplicate of this bug. ***
adding edt0.9.4+, please hit the 0.9.4 branch w/ this and mark w/ "fixed0.9.4"
upon landing.
Keywords: edt0.9.4+
*** Bug 115308 has been marked as a duplicate of this bug. ***
The shockwave installation/registration looks fine on windows trunk. *However*, 
3d groove(component) installation is giving out weird errors (like we used to 
get before)on MAC. While loading tank wars(mac only), I see an error message 
saying "3D GM error" just after I allow 3d groove to be installed on my machine. 
On mac(Alien X game), I get a message saying " 3D groove has run out of memory, 
pls restart application after increasing memory". Something is wrong, should I 
file another bug ? Pls suggest.. 
Looks like that happens after registration, during 3D Groove install, and only
on Mac, so I'd open another bug...assign it to me. Also, what's the buildID of
your Mac build?
filed bug 115696 for the 3d groove problem, build id for my mac was 2001121704.
ewww...looks like I need the nsIStringStream changes from bug 100172 on the
0.9.4 branch,...or do I?
Depends on: 100172
the nsIStringStream mods look like something we definitely want to avoid on the
0.9.4 branch. can we get around them?
NS_NewByteArrayInputStream equates to nsIStringInputStream::AdoptData
Thanks Darin. I tried this patch with Shockwave registration and it seems to do
the trick. It also passed the tester plugin I wrote. Can I get a review?
Comment on attachment 62115 [details] [diff] [review]
patch using byte input stream

sr=darin
Attachment #62115 - Flags: superreview+
Comment on attachment 62115 [details] [diff] [review]
patch using byte input stream

r=av
Attachment #62115 - Flags: review+
fixed0.9.4
Keywords: fixed0.9.4
Good to go! This is fixed on windows trunk/banch builds (1220). Everything works 
just fine. A seperate bug is already logged for the mac issue. VERIFIED.
Status: RESOLVED → VERIFIED
had not put the keywd 'verified0.9.4'. added that...
Keywords: verified0.9.4
Keywords: fixed0.9.4
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: