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

Status: VERIFIED FIXED

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

Description

[Peter Lubczynski]

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)]

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)]

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)]

This looks ugly, nominating for beta1.

Comment 4

[shrirang khanzode]

should we be closing bugscape 11600 ?

Comment 5

[rubydoo123]

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)]

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)]

Attachment: 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]

Andrei how does |nsIWebNavigation| fail? Does it not navigate or go back or you
can't QI to get it?

Comment 9

[av (gone)]

I get interface, call method, no error returns. It just does not do anything.

Comment 10

[Peter Lubczynski]

Can you use |gotoIndex| or the session history?

Adding keywords. This is a regression from the 0.9.4 branch tip.

Comment 11

[rubydoo123]

adding nsbeta1+

Comment 12

[av (gone)]

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)]

Attachment: stack trace mentioned by Radha in the previous comment

Comment 14

[lohphat]

Note: This does not depend on full-screen mode of the player nor the browser.

Comment 15

[lohphat]

*** Bug 136439 has been marked as a duplicate of this bug. ***

Comment 16

[av (gone)]

Updating title to avoid confusion.

Comment 17

[lohphat]

Could someone summarize the purpose and role npdsplay.dll plays and why removing
it shouldn't break something else?

Comment 18

[av (gone)]

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

[lohphat]

...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)]

I think our bug is that we display wrong page after plugin failure.

Comment 21

[lohphat]

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)]

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)]

By the way, the posted patch works just fine when I make a local test case with
bogus .asx file.

Comment 24

[lohphat]

I see the bug with valid content.  It doesn't have to be "bogus" for the paint
bug to appear.

Comment 25

[av (gone)]

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

[lohphat]

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)]

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)]

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

[lohphat]

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]

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]

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]

*** Bug 85722 has been marked as a duplicate of this bug. ***

Comment 33

[rubydoo123]

*** Bug 114746 has been marked as a duplicate of this bug. ***

Comment 34

[Peter Lubczynski]

*** Bug 143801 has been marked as a duplicate of this bug. ***

Comment 35

[av (gone)]

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]

*** Bug 129538 has been marked as a duplicate of this bug. ***

Comment 37

[Peter Lubczynski]

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]

mass move PL RTM 

Comment 39

[av (gone)]

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)]

Attachment: 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)]

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)]

Attachment: patch v.3

Thank you, serge, for suggestion. I think it can be made even more effective.
Please review.

Comment 43

[av (gone)]

Attachment: possible addition to the fix

Now I am wondering if we need this too.

Comment 44

[Peter Lubczynski]

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)]

Moving the patch to |MakeNew4xStreamInternal|, perhaps?

Comment 46

[av (gone)]

Attachment: 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)]

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]

Comment on attachment 86291 [details] [diff] [review]
patch v.4

r=peterl

Comment 49

[serge (gone)]

how about postURL from the local file c:\temp\bla\bla.bla
does it work with the patch?

Comment 50

[av (gone)]

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]

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)]

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)]

Attachment: 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]

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)]

Attachment: 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]

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]

*** Bug 149471 has been marked as a duplicate of this bug. ***

Comment 58

[Tuukka Tolvanen (sp3000)]

*** Bug 148149 has been marked as a duplicate of this bug. ***

Comment 59

[Jaime Rodriguez, Jr.]

what are the chances this is gonna be resolved by 06.14? pls add eta.

Comment 60

[Darin Fisher]

patch is ready... i just need to run some verification tests on it... i'm hoping
to do so tomorrow.

Comment 61

[Darin Fisher]

this patch passes all of our URL parsing tests.

Comment 62

[Darin Fisher]

Comment on attachment 86487 [details] [diff] [review]
patch v.6

sr=darin

Comment 63

[Doug Turner (:dougt)]

Comment on attachment 86487 [details] [diff] [review]
patch v.6

r=dougt

Comment 64

[Darin Fisher]

fixed-on-trunk

Comment 65

[Jaime Rodriguez, Jr.]

shrir - can you pls verify this one on the trunk tomorrow? thanks!

Comment 66

[shrirang khanzode]

first thing tomorrow...sure.

Comment 67

[shrirang khanzode]

wonderful, works great now. tested this test url and other testcases as well to 
be sure. verifi on 0620trunk.

Comment 68

[Susie Wyshak]

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

Comment 69

[Darin Fisher]

marking VERIFIED per previous two comments.

Comment 70

[Judson Valeski]

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.]

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]

fixed1.0.1

Comment 73

[rubydoo123]

*** Bug 151717 has been marked as a duplicate of this bug. ***

Comment 74

[shrirang khanzode]

verified fixd on 0723 brnch.