Last Comment Bug 133286 - Windows Media Player in full-page plugin mode may not repaint window
: Windows Media Player in full-page plugin mode may not repaint window
Status: VERIFIED FIXED
[ADT2 RTM][PL RTM],custrtm- [ETA 6/21]
: regression, topembed+
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: x86 Windows XP
: P2 normal (vote)
: mozilla1.0.1
Assigned To: Darin Fisher
: shrirang khanzode
Mentors:
http://www.windowsmedia.com/mg/Music.asp
: 85722 114746 129538 136439 143801 148149 149471 151717 (view as bug list)
Depends on:
Blocks: 143047
  Show dependency treegraph
 
Reported: 2002-03-25 09:10 PST by Peter Lubczynski
Modified: 2002-07-24 13:29 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v.1 -- not working yet (2.47 KB, patch)
2002-03-31 20:27 PST, av (gone)
no flags Details | Diff | Splinter Review
stack trace mentioned by Radha in the previous comment (2.30 KB, text/plain)
2002-04-08 16:03 PDT, av (gone)
no flags Details
patch v.2 (2.30 KB, patch)
2002-06-03 15:26 PDT, av (gone)
srgchrpv: review+
Details | Diff | Splinter Review
patch v.3 (1.40 KB, patch)
2002-06-04 14:12 PDT, av (gone)
no flags Details | Diff | Splinter Review
possible addition to the fix (969 bytes, text/plain)
2002-06-04 14:13 PDT, av (gone)
no flags Details
patch v.4 (3.52 KB, patch)
2002-06-04 15:01 PDT, av (gone)
peterlubczynski-bugs: review+
Details | Diff | Splinter Review
patch v.5 (2.02 KB, patch)
2002-06-05 14:44 PDT, av (gone)
no flags Details | Diff | Splinter Review
patch v.6 (2.22 KB, patch)
2002-06-05 15:25 PDT, av (gone)
dougt: review+
darin.moz: superreview+
jud: approval+
Details | Diff | Splinter Review

Description Peter Lubczynski 2002-03-25 09:10:45 PST
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?
Comment 1 av (gone) 2002-03-25 09:23:54 PST
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.
Comment 2 av (gone) 2002-03-25 09:30:43 PST
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.
Comment 3 av (gone) 2002-03-25 09:38:01 PST
This looks ugly, nominating for beta1.
Comment 4 shrirang khanzode 2002-03-25 10:08:39 PST
should we be closing bugscape 11600 ?
Comment 5 rubydoo123 2002-03-27 14:30:15 PST
sounds like there is a workaround - right? If the user selects the back button 
and then hits next -- will it render?
Comment 6 av (gone) 2002-03-27 17:10:18 PST
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.
Comment 7 av (gone) 2002-03-31 20:27:51 PST
Created attachment 76986 [details] [diff] [review]
patch v.1 -- not working yet

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?
Comment 8 Peter Lubczynski 2002-04-01 09:01:14 PST
Andrei how does |nsIWebNavigation| fail? Does it not navigate or go back or you
can't QI to get it?
Comment 9 av (gone) 2002-04-01 10:06:50 PST
I get interface, call method, no error returns. It just does not do anything.
Comment 10 Peter Lubczynski 2002-04-02 15:49:11 PST
Can you use |gotoIndex| or the session history?

Adding keywords. This is a regression from the 0.9.4 branch tip.
Comment 11 rubydoo123 2002-04-03 09:33:23 PST
adding nsbeta1+
Comment 12 av (gone) 2002-04-08 16:01:00 PDT
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 av (gone) 2002-04-08 16:03:05 PDT
Created attachment 78252 [details]
stack trace mentioned by Radha in the previous comment
Comment 14 Derwood 2002-04-09 14:32:48 PDT
Note: This does not depend on full-screen mode of the player nor the browser.
Comment 15 Derwood 2002-04-09 14:34:24 PDT
*** Bug 136439 has been marked as a duplicate of this bug. ***
Comment 16 av (gone) 2002-04-09 14:55:38 PDT
Updating title to avoid confusion.
Comment 17 Derwood 2002-04-10 02:06:51 PDT
Could someone summarize the purpose and role npdsplay.dll plays and why removing
it shouldn't break something else?
Comment 18 av (gone) 2002-04-10 09:07:44 PDT
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 Derwood 2002-04-10 09:29:44 PDT
...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 av (gone) 2002-04-10 10:28:05 PDT
I think our bug is that we display wrong page after plugin failure.
Comment 21 Derwood 2002-04-10 10:42:43 PDT
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 av (gone) 2002-04-10 12:57:31 PDT
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 av (gone) 2002-04-10 12:58:47 PDT
By the way, the posted patch works just fine when I make a local test case with
bogus .asx file.
Comment 24 Derwood 2002-04-10 13:11:36 PDT
I see the bug with valid content.  It doesn't have to be "bogus" for the paint
bug to appear.
Comment 25 av (gone) 2002-04-10 13:34:09 PDT
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 Derwood 2002-04-10 13:45:26 PDT
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 av (gone) 2002-04-10 14:06:37 PDT
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 saari (gone) 2002-04-11 12:54:56 PDT
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 Derwood 2002-04-11 14:04:21 PDT
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 rubydoo123 2002-04-15 15:40:35 PDT
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.
Comment 31 David G King 2002-04-20 17:14:08 PDT
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 Olivier Cahagne 2002-04-21 02:58:12 PDT
*** Bug 85722 has been marked as a duplicate of this bug. ***
Comment 33 rubydoo123 2002-04-22 15:51:28 PDT
*** Bug 114746 has been marked as a duplicate of this bug. ***
Comment 34 Peter Lubczynski 2002-05-11 21:01:21 PDT
*** Bug 143801 has been marked as a duplicate of this bug. ***
Comment 35 av (gone) 2002-05-11 21:50:33 PDT
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
Comment 36 rubydoo123 2002-05-20 15:24:49 PDT
*** Bug 129538 has been marked as a duplicate of this bug. ***
Comment 37 Peter Lubczynski 2002-05-23 08:37:23 PDT
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.
Comment 38 rubydoo123 2002-05-30 17:25:42 PDT
mass move PL RTM 
Comment 39 av (gone) 2002-06-03 13:37:29 PDT
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.
Comment 40 av (gone) 2002-06-03 15:26:31 PDT
Created attachment 86122 [details] [diff] [review]
patch v.2

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 serge (gone) 2002-06-03 16:29:42 PDT
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.
Comment 42 av (gone) 2002-06-04 14:12:08 PDT
Created attachment 86282 [details] [diff] [review]
patch v.3

Thank you, serge, for suggestion. I think it can be made even more effective.
Please review.
Comment 43 av (gone) 2002-06-04 14:13:37 PDT
Created attachment 86283 [details]
possible addition to the fix

Now I am wondering if we need this too.
Comment 44 Peter Lubczynski 2002-06-04 14:23:40 PDT
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 av (gone) 2002-06-04 14:37:53 PDT
Moving the patch to |MakeNew4xStreamInternal|, perhaps?
Comment 46 av (gone) 2002-06-04 15:01:11 PDT
Created attachment 86291 [details] [diff] [review]
patch v.4

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 serge (gone) 2002-06-04 18:01:07 PDT
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
Comment 48 Peter Lubczynski 2002-06-04 19:27:00 PDT
Comment on attachment 86291 [details] [diff] [review]
patch v.4

r=peterl
Comment 49 serge (gone) 2002-06-05 09:55:04 PDT
how about postURL from the local file c:\temp\bla\bla.bla
does it work with the patch?
Comment 50 av (gone) 2002-06-05 10:49:34 PDT
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 51 Darin Fisher 2002-06-05 11:01:07 PDT
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.
Comment 52 av (gone) 2002-06-05 13:29:59 PDT
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 av (gone) 2002-06-05 14:44:02 PDT
Created attachment 86477 [details] [diff] [review]
patch v.5

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.
Comment 54 Darin Fisher 2002-06-05 15:12:14 PDT
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;  
   }
Comment 55 av (gone) 2002-06-05 15:25:37 PDT
Created attachment 86487 [details] [diff] [review]
patch v.6

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.
Comment 56 Darin Fisher 2002-06-05 15:40:16 PDT
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.
Comment 57 Peter Lubczynski 2002-06-05 18:34:32 PDT
*** Bug 149471 has been marked as a duplicate of this bug. ***
Comment 58 Tuukka Tolvanen (sp3000) 2002-06-07 13:00:28 PDT
*** Bug 148149 has been marked as a duplicate of this bug. ***
Comment 59 Jaime Rodriguez, Jr. 2002-06-08 10:15:19 PDT
what are the chances this is gonna be resolved by 06.14? pls add eta.
Comment 60 Darin Fisher 2002-06-12 22:05:09 PDT
patch is ready... i just need to run some verification tests on it... i'm hoping
to do so tomorrow.
Comment 61 Darin Fisher 2002-06-18 13:51:08 PDT
this patch passes all of our URL parsing tests.
Comment 62 Darin Fisher 2002-06-18 13:52:04 PDT
Comment on attachment 86487 [details] [diff] [review]
patch v.6

sr=darin
Comment 63 Doug Turner (:dougt) 2002-06-18 13:55:48 PDT
Comment on attachment 86487 [details] [diff] [review]
patch v.6

r=dougt
Comment 64 Darin Fisher 2002-06-18 14:29:55 PDT
fixed-on-trunk
Comment 65 Jaime Rodriguez, Jr. 2002-06-18 17:36:26 PDT
shrir - can you pls verify this one on the trunk tomorrow? thanks!
Comment 66 shrirang khanzode 2002-06-18 17:37:59 PDT
first thing tomorrow...sure.
Comment 67 shrirang khanzode 2002-06-20 11:12:36 PDT
wonderful, works great now. tested this test url and other testcases as well to 
be sure. verifi on 0620trunk.
Comment 69 Darin Fisher 2002-06-20 11:42:40 PDT
marking VERIFIED per previous two comments.
Comment 70 Judson Valeski 2002-06-20 16:25:51 PDT
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.
Comment 71 Jaime Rodriguez, Jr. 2002-06-20 20:46:38 PDT
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.
Comment 72 Darin Fisher 2002-06-21 16:45:27 PDT
fixed1.0.1
Comment 73 rubydoo123 2002-07-18 14:11:03 PDT
*** Bug 151717 has been marked as a duplicate of this bug. ***
Comment 74 shrirang khanzode 2002-07-24 13:29:20 PDT
verified fixd on 0723 brnch.

Note You need to log in before you can comment on or make changes to this bug.