Closed
Bug 133286
Opened 22 years ago
Closed 22 years ago
Windows Media Player in full-page plugin mode may not repaint window
Categories
(Core Graveyard :: Plug-ins, defect, P2)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.0.1
People
(Reporter: peterlubczynski-bugs, Assigned: darin.moz)
References
()
Details
(Keywords: regression, topembed+, Whiteboard: [ADT2 RTM][PL RTM],custrtm- [ETA 6/21])
Attachments
(2 files, 6 obsolete files)
2.30 KB,
text/plain
|
Details | |
2.22 KB,
patch
|
dougt
:
review+
darin.moz
:
superreview+
jud
:
approval+
|
Details | Diff | Splinter Review |
1. Go to testcase and click on any .asx link like "56k,100k, 300k". 2. The full-page plugin causes Windows media player to pop up in a helper application. 3. Notice the browser window no longer repaints itself but still shows parts of the last page. It looks bad and is unsable until you click back button. set MOZ_PLUGIN_PATH=C:\Program Files\Windows Media Player npdsplay: 3.0.2.627 WMP 8.00.00.4477 (but also seen on 7.x) Andrei, I think we've had a bug like this before? Was it ever resolved?
This was bugscape 11600 and has been fixed on the branch. I did not pay much attention to this bug on the trunk after at some point it just started work for me. I don't remember if it has been filed on Bugzilla. Anyway, if you see it on the trunk again, we should fix it. Taking.
Assignee: beppe → av
Reproducable on Win2000. To see it one can just past the .asx url in the url bar. And if you don't have the plugin (npdsplay.dll) in the plugins folder, everything works just fine.
This looks ugly, nominating for beta1.
Comment 4•22 years ago
|
||
should we be closing bugscape 11600 ?
Comment 5•22 years ago
|
||
sounds like there is a workaround - right? If the user selects the back button and then hits next -- will it render?
I think it will. But first let me try similar approach I used for the branch. If it works, it is going to be not so hard to fix it.
I am trying to utilize the similar approach which I used to fix bugscape issue 11600. This patch is pretty straightforward and I can see the error condition on both Win2000 and WinXP, so it should be good for both. Strangely enough, the |nsIWebNavigation| service just does not work for me in this patch. I am apparantely overlooking something obvious but cannot immediately see what. Peter, can you please take a look?
Reporter | ||
Comment 8•22 years ago
|
||
Andrei how does |nsIWebNavigation| fail? Does it not navigate or go back or you can't QI to get it?
I get interface, call method, no error returns. It just does not do anything.
Reporter | ||
Comment 10•22 years ago
|
||
Can you use |gotoIndex| or the session history? Adding keywords. This is a regression from the 0.9.4 branch tip.
Keywords: regression,
topembed
Comment 11•22 years ago
|
||
adding nsbeta1+
Comment 12•22 years ago
|
||
Interesting finding (thanks, Radha!): "The goback() operation is succeeding. And the load of the previous page starts. But the load is cancelled by the load of "file://c:\docume~1\radha~1.met\locals~1\temp\wmpc4e.htm/" at the following stack trace. I think this is the actual file downloaded by the media player. It looks like someone in another thread is posting an event to load this page, thereby the previous page is nevercompletely rendered."
Comment 13•22 years ago
|
||
Updated•22 years ago
|
Comment 14•22 years ago
|
||
Note: This does not depend on full-screen mode of the player nor the browser.
Comment 15•22 years ago
|
||
*** Bug 136439 has been marked as a duplicate of this bug. ***
Comment 16•22 years ago
|
||
Updating title to avoid confusion.
Summary: Windows Media Player in full-screen mode may not repaint window → Windows Media Player in full-page plugin mode may not repaint window
Comment 17•22 years ago
|
||
Could someone summarize the purpose and role npdsplay.dll plays and why removing it shouldn't break something else?
Comment 18•22 years ago
|
||
Media Player is an application to play various media formats and it can be hooked in other applications and processes. npdsplay.dll is supposed to launch Media Player as a plugin, which means that all the gui, controls etc of the player should appear inside the browser window either embedded in other page content (if we have <embed> tag) or full-page (like in our case). Another mechanism of playing media out of the web using Media Player is employing it as a helper application when it takes over completely and launches its own window with control in it. This should normally happen if we don't have a plugin but the OS has some application registered to handle this given mimetype. In our test case Media Player tries to starts as a plugin but cannot finish for some reason, aborts and starts as a helper application instead, leaving incomplete page uncleaned. This is the actual bug. If we remove npdsplay.dll it will not attempt to start as a plugin and launch helper app at once, thus the problem does not occur.
Comment 19•22 years ago
|
||
...but without nsdsplay.dll, imbedded WMP content cannot play at all. So, is the bug in the .dll or the plug-in code?
Comment 20•22 years ago
|
||
I think our bug is that we display wrong page after plugin failure.
Comment 21•22 years ago
|
||
Well, it's not plug-in failure that shows the bug, just simply lauching the plug-in causes it. I cannot access the page while the player has launched and is playing the correct content.
Comment 22•22 years ago
|
||
I do not fully understand what you are saying. What I mean, the plugin fails to play the content, specifically opening the stream fails, after which the dll decides to launch Media Player as a helper application. The page stays in its intermediate state, which I beleive is our bug.
Comment 23•22 years ago
|
||
By the way, the posted patch works just fine when I make a local test case with bogus .asx file.
Comment 24•22 years ago
|
||
I see the bug with valid content. It doesn't have to be "bogus" for the paint bug to appear.
Comment 25•22 years ago
|
||
Content might be valid but plugin may still fail. As I said, it gets error code when trying to open a stream via NPAPI. This could be a bug too, but the plugin itself seems to handle this situation well. Can you please make your point?
Comment 26•22 years ago
|
||
I'm just trying to clarify the comments here -- you're referring to "failure" and "bogus content" when I can reporduce with valid content. The terminology, the summary are incorrect. The bug shows itself in imbedded (not just full-page as the summary states) and with valid content (as opposed to "bogus" content as you claim).
Comment 27•22 years ago
|
||
1. plugin does fail to play content, helper application does not 2. my comment about 'bogus' content was just a description of what I did to see the patch working 3. can you show the bug in embedded case? I did not see it. 4. please suggest the correct terminology and your version of summary.
Comment 28•22 years ago
|
||
topembed+ since it is something we need for an embedding customer, assuming we can fix this on our side and it is our bug.
Comment 29•22 years ago
|
||
OK, I admit it, I could have been on crack. I've been trying to reproduce the bug in embedded mode and can't.
Comment 30•22 years ago
|
||
I was able to reproduce this one quite easily using the build from today - 20020415. Try this (this one uses media player): 1. Go to http://hollywoodandvine.com/ 2. In the drop down list, where it says select a channel, select John Lennon 3. When the resulting page finally loads, select 'Play All' from one of the music choice. Windows Media player will launch, grab the plug-in and move it over the browser window -- it's awful Now, minimize both windows (browser and plug-in), restore the browser window and go to another page, to force a repaint. Restore the plug-in window and move over the browser window, The painting problem will not occur.
Updated•22 years ago
|
Whiteboard: [ADT2+]
Updated•22 years ago
|
Whiteboard: [ADT2+] → [ADT2]
Comment 31•22 years ago
|
||
After seeing this problem with 1.0RC1, I found this bug, and Bug #85722 (which appears to be a dup of this one).
Comment 32•22 years ago
|
||
*** Bug 85722 has been marked as a duplicate of this bug. ***
Comment 33•22 years ago
|
||
*** Bug 114746 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 34•22 years ago
|
||
*** Bug 143801 has been marked as a duplicate of this bug. ***
Comment 35•22 years ago
|
||
I found a generic way to cause the similar symptom. Very likely it is related. 1. load a full-page plugin, say, Flash: http://bugzilla.mozilla.org/attachment.cgi?id=83058&action=view 2. remove/rename npswf32.dll while it is running (this is possible at least on XP) 3. hit reload button, the page becomes non-repaintable
Updated•22 years ago
|
Whiteboard: [ADT2] → [ADT2][PL RTM]
Comment 36•22 years ago
|
||
*** Bug 129538 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 37•22 years ago
|
||
removing adt1.0.0, this bug does not have a FIX yet and it not ready to be nominated for checkin to the 1.0 branch. btw, I am seeing the URL in comment #12. Looking at this temporary file created by WMP, it looks like the plugin is trying to use Javascript and do the right thing by trying to tell us to go back to the previous page: <html><script language='javascript'>window.history.go(-2);</script></html> However, what I see is that the we can not load this file because we can't find it because of the last '/'. We add that -- it does not come from the plugin. The format of the URL that the plugin passes us is like in comment #12.
Keywords: adt1.0.0
Whiteboard: [ADT2+ RTM][PL RTM] → [ADT2 RTM][PL RTM]
Comment 39•22 years ago
|
||
Comment on attachment 76986 [details] [diff] [review] patch v.1 -- not working yet This patch is not relevant. The problem is that WMP gives us the URL which is formatted not exactly how Necko expects. Thanks to darin for pointing this out.
Attachment #76986 -
Attachment is obsolete: true
Comment 40•22 years ago
|
||
This is a simple prototype which illustrates a possible approach to fix the bug. And it really does. Maybe this patch is even good enough to get in :) Please review.
Comment 41•22 years ago
|
||
Comment on attachment 86122 [details] [diff] [review] patch v.2 > char* absURIStr; >- NS_MakeAbsoluteURI(&absURIStr, aURL, uri); we cannot continue if NS_MakeAbsoluteURI() fails so I would change this code rv = NS_MakeAbsoluteURI(&absURIStr, aURL, uri); NS_ENSURE_SUCCESS(rv, rv); >+ // let's fix possible back slashes as necko cannot handle this >+ // while some plugins may use them, see bug 133286 and start scan if there is '\'char for (char * p = strchr(absURIStr); *p; p++) { >+ for (char * p = absURIStr; *p; p++) { and there could be a problem with japanese charset as I found it in http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsIOServiceOS2.cpp#59 with that we have to use isleadbyte // We need to call isleadbyte because // Japanese windows can have 0x5C in the sencond byte // of a Japanese character, for example 0x8F 0x5C is // one Japanese character if(isleadbyte(*p) && *(p+1)) ++p; else if (*p == '\\') *p = '/'; other then that, we need this patch, r=serge.
Attachment #86122 -
Flags: review+
Comment 42•22 years ago
|
||
Thank you, serge, for suggestion. I think it can be made even more effective. Please review.
Attachment #86122 -
Attachment is obsolete: true
Comment 43•22 years ago
|
||
Now I am wondering if we need this too.
Reporter | ||
Comment 44•22 years ago
|
||
Maybe I'm missing something here, but what about when target=NULL and we don't go through layout but rather |nsPluginHostImpl::NewPluginURLStream|? Since nsIURI doesn't do this fixup, I wonder if we will be breaking anything here?
Comment 45•22 years ago
|
||
Moving the patch to |MakeNew4xStreamInternal|, perhaps?
Comment 46•22 years ago
|
||
Like this... In fact, with this patch, there is no need to make separate identical changes to both |nsIPluginInstanceOwner| objects, and it covers pretty much every path we can take from the plugin to the browser.
Comment 47•22 years ago
|
||
well, I just tested http://lxr.mozilla.org/seamonkey/source/modules/plugin/tools/tester/testcase/testapi.html geturl works just fine on file://l:\publish\bugzilla\115308\readme.txt
Reporter | ||
Comment 48•22 years ago
|
||
Comment on attachment 86291 [details] [diff] [review] patch v.4 r=peterl
Attachment #86291 -
Flags: review+
Attachment #86282 -
Attachment is obsolete: true
Attachment #86283 -
Attachment is obsolete: true
Comment 49•22 years ago
|
||
how about postURL from the local file c:\temp\bla\bla.bla does it work with the patch?
Comment 50•22 years ago
|
||
The patch fixes the url to post to, not the data we are going to post. So if Mozilla ever supports posting to file:// type url we should test it.
Assignee | ||
Comment 51•22 years ago
|
||
Comment on attachment 86291 [details] [diff] [review] patch v.4 >Index: ns4xPlugin.cpp >+ // let's fix possible back slashes in the url as necko cannot handle >+ // this while some plugins may use them, see bug 133286 >+ // but don't touch anything if following local language 'lead byte' >+ char * relURL = PL_strdup(relativeURL); nit: backslashes are uncommon right? so how about deferring this strdup until after we know for certain that the string contains backslashes? >+ if (!relURL) >+ return NPERR_GENERIC_ERROR; >+ >+ char *p = relURL; >+ while (p = PL_strchr(p, '\\')) { >+ if (!isleadbyte(*(p - 1)) || (p == relURL)) >+ *p = '/'; >+ >+ p++; >+ } major: what if the first character in relURL is a backslash? looks to me like the call to isleadbyte would address invalid memory. you could just move the (p == relURL) test in front of the isleadbyte call to avoid this problem.
Attachment #86291 -
Flags: needs-work+
Comment 52•22 years ago
|
||
Although the patch does fix the problem, looks like this is not we really want to do. Backslashes can be legitimate parts of the url string, so changing them we are going to break cases with querry strings or passwords. Another wrong thing with this particular plugin, as serge pointed out, is that it uses 'file://' as a uri scheme, note only two slashes. Changing it to three also seems to fix the problem and is probably the correct way to do that.
Comment 53•22 years ago
|
||
Very basic thing, we might benefit if we fix it in Necko. This patch does not limit existing Windows url authoring code to the presence of forward slashes after the scheme.
Attachment #86291 -
Attachment is obsolete: true
Assignee | ||
Comment 54•22 years ago
|
||
Comment on attachment 86477 [details] [diff] [review] patch v.5 >Index: nsURLParsers.cpp > const char *p = nsnull; > if (specLen > 2) { > // looks like there is an authority section >- p = (const char *) memchr(spec + 2, '/', specLen - 2); > #if defined(XP_PC) > // if the authority looks like a drive number then we > // really want to treat it as part of the path >- if (((p && (p - (spec + 2) == 2)) || (specLen == 4)) && >- (spec[3] == ':' || spec[3] == '|') && >+ if ((spec[3] == ':' || spec[3] == '|') && > (nsCRT::IsAsciiAlpha(spec[2]))) { > pos = 1; > break; > } > #endif >+ p = (const char *) memchr(spec + 2, '/', specLen - 2); > } a few problems... |spec| is not necessarily null terminated. so, if specLen == 3, then spec[3] would address invalid memory. another problem... suppose spec = "//f:4000/bar" in this case "f" is probably a hostname and not a drive letter. that's why the old code checked for '/' before checking for the existance of a drive letter. how about this check: if ((specLen > 3) && (spec[3] == ':' || spec[3] == '|') && nsCRT::IsAsciiAlpha(spec[2]) && ((specLen == 4) || (spec[4] == '/') || (spec[4] == '\\'))) { pos = 1; break; }
Attachment #86477 -
Flags: needs-work+
Comment 55•22 years ago
|
||
darin, if you agree that we can go ahead and fix this problem in Necko and given you probably know better than anybody else how to test this code, do you think I should reassign this to you? It is fine with me either way.
Attachment #86477 -
Attachment is obsolete: true
Assignee | ||
Comment 56•22 years ago
|
||
yeah, i can take it from here... i have a set of testcases that i'll need to run to verify that this doesn't regress anything.
Assignee: av → darin
Status: ASSIGNED → NEW
Reporter | ||
Comment 57•22 years ago
|
||
*** Bug 149471 has been marked as a duplicate of this bug. ***
Comment 58•22 years ago
|
||
*** Bug 148149 has been marked as a duplicate of this bug. ***
Comment 59•22 years ago
|
||
what are the chances this is gonna be resolved by 06.14? pls add eta.
Whiteboard: [ADT2 RTM][PL RTM],custrtm- → [ADT2 RTM][PL RTM],custrtm- [ETA Needed]
Assignee | ||
Comment 60•22 years ago
|
||
patch is ready... i just need to run some verification tests on it... i'm hoping to do so tomorrow.
Status: NEW → ASSIGNED
Whiteboard: [ADT2 RTM][PL RTM],custrtm- [ETA Needed] → [ADT2 RTM][PL RTM],custrtm- [ETA 6/7]
Assignee | ||
Comment 61•22 years ago
|
||
this patch passes all of our URL parsing tests.
Assignee | ||
Comment 62•22 years ago
|
||
Comment on attachment 86487 [details] [diff] [review] patch v.6 sr=darin
Attachment #86487 -
Flags: superreview+
Comment 63•22 years ago
|
||
Comment on attachment 86487 [details] [diff] [review] patch v.6 r=dougt
Attachment #86487 -
Flags: review+
Assignee | ||
Comment 64•22 years ago
|
||
fixed-on-trunk
Comment 65•22 years ago
|
||
shrir - can you pls verify this one on the trunk tomorrow? thanks!
Keywords: adt1.0.1
Whiteboard: [ADT2 RTM][PL RTM],custrtm- [ETA 6/7] → [ADT2 RTM][PL RTM],custrtm- [ETA 6/21]
Comment 66•22 years ago
|
||
first thing tomorrow...sure.
Comment 67•22 years ago
|
||
wonderful, works great now. tested this test url and other testcases as well to be sure. verifi on 0620trunk.
Comment 68•22 years ago
|
||
wfm me also on 6/20 trunk with http://www2.coca-cola.com/presscenter/av_advertising.html http://www.wqxr.com/cgi-bin/iowa/air/listen/index.html
Assignee | ||
Comment 69•22 years ago
|
||
marking VERIFIED per previous two comments.
Status: RESOLVED → VERIFIED
Updated•22 years ago
|
Attachment #86487 -
Flags: approval+
Comment 70•22 years ago
|
||
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+" keyword and add the "fixed1.0.1" keyword.
Keywords: mozilla1.0.1 → mozilla1.0.1+
Comment 71•22 years ago
|
||
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 the "fixed1.0.1" keyword.
Updated•22 years ago
|
Comment 73•22 years ago
|
||
*** Bug 151717 has been marked as a duplicate of this bug. ***
Updated•1 year ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•