Closed
Bug 108966
Opened 23 years ago
Closed 23 years ago
shockwave player installation does not complete
Categories
(Core Graveyard :: Plug-ins, defect, P1)
Core Graveyard
Plug-ins
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9.7
People
(Reporter: shrir, Assigned: peterlubczynski-bugs)
References
()
Details
(Keywords: regression)
Attachments
(3 files, 2 obsolete files)
53.50 KB,
application/x-zip-compressed
|
Details | |
20.46 KB,
patch
|
peterlubczynski-bugs
:
review+
peterlubczynski-bugs
:
superreview+
|
Details | Diff | Splinter Review |
17.50 KB,
patch
|
serhunt
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 3•23 years ago
|
||
seems like after 6.2 got released..wil try to find out the date when this happened...
Assignee | ||
Comment 4•23 years ago
|
||
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.
Assignee | ||
Comment 5•23 years ago
|
||
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. :(
Comment 6•23 years ago
|
||
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.
Comment 7•23 years ago
|
||
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.
Assignee | ||
Comment 8•23 years ago
|
||
Cool! I think that'll work. I'll try it first thing tomorrow.
Assignee | ||
Comment 9•23 years ago
|
||
This seems to fix the Shockwave registration. I still need to a little more testing...
Assignee | ||
Updated•23 years ago
|
Assignee | ||
Comment 10•23 years ago
|
||
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
Comment 11•23 years ago
|
||
Please interact with Peter about testing this with a test driver.
Assignee | ||
Comment 12•23 years ago
|
||
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 13•23 years ago
|
||
Comment on attachment 60648 [details] [diff] [review] updated patch Looks good to me. r=av
Attachment #60648 -
Flags: review+
Assignee | ||
Comment 14•23 years ago
|
||
Darin or Scott, can I get your super review? Thanks.
Keywords: edt0.9.4
Comment 15•23 years ago
|
||
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+
Comment 16•23 years ago
|
||
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?
Assignee | ||
Comment 17•23 years ago
|
||
Attachment #60648 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #61321 -
Flags: superreview+
Attachment #61321 -
Flags: review+
Assignee | ||
Comment 18•23 years ago
|
||
Patch in trunk, marking FIXED. I'll renomiate after some baking....
Assignee | ||
Comment 19•23 years ago
|
||
*** Bug 113543 has been marked as a duplicate of this bug. ***
Comment 20•23 years ago
|
||
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+
Assignee | ||
Comment 21•23 years ago
|
||
*** Bug 115308 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 22•23 years ago
|
||
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..
Assignee | ||
Comment 23•23 years ago
|
||
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?
Reporter | ||
Comment 24•23 years ago
|
||
filed bug 115696 for the 3d groove problem, build id for my mac was 2001121704.
Assignee | ||
Comment 25•23 years ago
|
||
ewww...looks like I need the nsIStringStream changes from bug 100172 on the 0.9.4 branch,...or do I?
Depends on: 100172
Comment 26•23 years ago
|
||
the nsIStringStream mods look like something we definitely want to avoid on the 0.9.4 branch. can we get around them?
Comment 27•23 years ago
|
||
NS_NewByteArrayInputStream equates to nsIStringInputStream::AdoptData
Assignee | ||
Comment 28•23 years ago
|
||
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 29•23 years ago
|
||
Comment on attachment 62115 [details] [diff] [review] patch using byte input stream sr=darin
Attachment #62115 -
Flags: superreview+
Comment 30•23 years ago
|
||
Comment on attachment 62115 [details] [diff] [review] patch using byte input stream r=av
Attachment #62115 -
Flags: review+
Reporter | ||
Comment 32•23 years ago
|
||
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
Reporter | ||
Comment 33•23 years ago
|
||
had not put the keywd 'verified0.9.4'. added that...
Keywords: verified0.9.4
Keywords: fixed0.9.4
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•