Closed Bug 133286 Opened 23 years ago Closed 22 years ago

Windows Media Player in full-page plugin mode may not repaint window


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

Windows XP


(Not tracked)



(Reporter: peterlubczynski-bugs, Assigned: darin.moz)




(Keywords: regression, topembed+, Whiteboard: [ADT2 RTM][PL RTM],custrtm- [ETA 6/21])


(2 files, 6 obsolete files)

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
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
WMP (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.
Keywords: nsbeta1
Priority: -- → P2
Target Milestone: --- → mozilla1.0
should we be closing bugscape 11600 ?
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.
Attached patch patch v.1 -- not working yet (obsolete) — Splinter Review
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?
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.
Can you use |gotoIndex| or the session history?

Adding keywords. This is a regression from the 0.9.4 branch tip.
Keywords: regression, topembed
adding nsbeta1+
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."
Keywords: nsbeta1nsbeta1+
Note: This does not depend on full-screen mode of the player nor the browser.
*** Bug 136439 has been marked as a duplicate of this bug. ***
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
Could someone summarize the purpose and role npdsplay.dll plays and why removing
it shouldn't break something else?
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.
...but without nsdsplay.dll, imbedded WMP content cannot play at all.

So, is the bug in the .dll or the plug-in code?
I think our bug is that we display wrong page after plugin failure.
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.
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.
By the way, the posted patch works just fine when I make a local test case with
bogus .asx file.
I see the bug with valid content.  It doesn't have to be "bogus" for the paint
bug to appear.
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?
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).
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.
topembed+ since it is something we need for an embedding customer, assuming we
can fix this on our side and it is our bug.
Keywords: topembedtopembed+
OK, I admit it, I could have been on crack.  I've been trying to reproduce the
bug in embedded mode and can't.
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
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.
Whiteboard: [ADT2+]
Whiteboard: [ADT2+] → [ADT2]
After seeing this problem with 1.0RC1, I found this bug, and Bug #85722 (which
appears to be a dup of this one).
*** Bug 85722 has been marked as a duplicate of this bug. ***
*** Bug 114746 has been marked as a duplicate of this bug. ***
*** Bug 143801 has been marked as a duplicate of this bug. ***
I found a generic way to cause the similar symptom. Very likely it is related.

1. load a full-page plugin, say, Flash:

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
Whiteboard: [ADT2] → [ADT2][PL RTM]
*** Bug 129538 has been marked as a duplicate of this bug. ***
Keywords: adt1.0.0
Whiteboard: [ADT2][PL RTM] → [ADT2+ RTM][PL RTM]
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]
Whiteboard: [ADT2 RTM][PL RTM] → [ADT2 RTM][PL RTM],custrtm-
mass move PL RTM 
Target Milestone: mozilla1.0 → mozilla1.0.1
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
Attached patch patch v.2 (obsolete) — Splinter Review
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 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);

>+    // 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
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))
      else if (*p == '\\')
	*p = '/';

other then that, we need this patch, r=serge.
Attachment #86122 - Flags: review+
Attached patch patch v.3 (obsolete) — Splinter Review
Thank you, serge, for suggestion. I think it can be made even more effective.
Please review.
Attachment #86122 - Attachment is obsolete: true
Attached file possible addition to the fix (obsolete) —
Now I am wondering if we need this too.
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?
Moving the patch to |MakeNew4xStreamInternal|, perhaps?
Attached patch patch v.4 (obsolete) — Splinter Review
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.
well, I just tested
geturl works just fine on file://l:\publish\bugzilla\115308\readme.txt
Comment on attachment 86291 [details] [diff] [review]
patch v.4

Attachment #86291 - Flags: review+
Attachment #86282 - Attachment is obsolete: true
Attachment #86283 - Attachment is obsolete: true
how about postURL from the local file c:\temp\bla\bla.bla
does it work with the patch?
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.
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)
>+  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+
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.
Attached patch patch v.5 (obsolete) — Splinter Review
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
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;
Attachment #86477 - Flags: needs-work+
Attached patch patch v.6Splinter Review
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
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
*** Bug 149471 has been marked as a duplicate of this bug. ***
*** Bug 148149 has been marked as a duplicate of this bug. ***
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]
patch is ready... i just need to run some verification tests on it... i'm hoping
to do so tomorrow.
Whiteboard: [ADT2 RTM][PL RTM],custrtm- [ETA Needed] → [ADT2 RTM][PL RTM],custrtm- [ETA 6/7]
this patch passes all of our URL parsing tests.
Comment on attachment 86487 [details] [diff] [review]
patch v.6

Attachment #86487 - Flags: superreview+
Comment on attachment 86487 [details] [diff] [review]
patch v.6

Attachment #86487 - Flags: review+
Closed: 22 years ago
Keywords: mozilla1.0.1
Resolution: --- → FIXED
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]
first thing tomorrow...sure.
wonderful, works great now. tested this test url and other testcases as well to 
be sure. verifi on 0620trunk.
marking VERIFIED per previous two comments.
Attachment #86487 - Flags: approval+
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.
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.
Blocks: 143047
Keywords: adt1.0.1adt1.0.1+
*** Bug 151717 has been marked as a duplicate of this bug. ***
verified fixd on 0723 brnch.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.