The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla1.0.1

Status

()

Core
Plug-ins
P2
normal
VERIFIED FIXED
15 years ago
15 years ago

People

(Reporter: Peter Lubczynski, Assigned: Darin Fisher)

Tracking

({regression, topembed+})

Trunk
mozilla1.0.1
x86
Windows XP
regression, topembed+
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ADT2 RTM][PL RTM],custrtm- [ETA 6/21], URL)

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

15 years ago
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

15 years ago
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

Comment 2

15 years ago
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

15 years ago
This looks ugly, nominating for beta1.
Status: NEW → ASSIGNED
Keywords: nsbeta1
Priority: -- → P2
Target Milestone: --- → mozilla1.0

Comment 4

15 years ago
should we be closing bugscape 11600 ?

Comment 5

15 years ago
sounds like there is a workaround - right? If the user selects the back button 
and then hits next -- will it render?

Comment 6

15 years ago
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

15 years ago
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?
(Reporter)

Comment 8

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

Comment 9

15 years ago
I get interface, call method, no error returns. It just does not do anything.
(Reporter)

Comment 10

15 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

15 years ago
adding nsbeta1+

Comment 12

15 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

15 years ago
Created attachment 78252 [details]
stack trace mentioned by Radha in the previous comment

Updated

15 years ago
Keywords: nsbeta1 → nsbeta1+

Comment 14

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

Comment 15

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

Comment 16

15 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

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

Comment 18

15 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

15 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

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

Comment 21

15 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

15 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

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

Comment 24

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

Comment 25

15 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

15 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

15 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

15 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.
Keywords: topembed → topembed+

Comment 29

15 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

15 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

15 years ago
Whiteboard: [ADT2+]

Updated

15 years ago
Whiteboard: [ADT2+] → [ADT2]

Comment 31

15 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

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

Comment 33

15 years ago
*** Bug 114746 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 34

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

Comment 35

15 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

15 years ago
Whiteboard: [ADT2] → [ADT2][PL RTM]

Comment 36

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

Updated

15 years ago
Keywords: adt1.0.0
Whiteboard: [ADT2][PL RTM] → [ADT2+ RTM][PL RTM]
(Reporter)

Comment 37

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

Updated

15 years ago
Whiteboard: [ADT2 RTM][PL RTM] → [ADT2 RTM][PL RTM],custrtm-

Comment 38

15 years ago
mass move PL RTM 
Target Milestone: mozilla1.0 → mozilla1.0.1

Comment 39

15 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

15 years ago
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

15 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

15 years ago
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.
Attachment #86122 - Attachment is obsolete: true

Comment 43

15 years ago
Created attachment 86283 [details]
possible addition to the fix

Now I am wondering if we need this too.
(Reporter)

Comment 44

15 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

15 years ago
Moving the patch to |MakeNew4xStreamInternal|, perhaps?

Comment 46

15 years ago
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

15 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

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

r=peterl
Attachment #86291 - Flags: review+

Updated

15 years ago
Attachment #86282 - Attachment is obsolete: true

Updated

15 years ago
Attachment #86283 - Attachment is obsolete: true

Comment 49

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

Comment 50

15 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

15 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

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

Updated

15 years ago

Comment 53

15 years ago
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.
Attachment #86291 - Attachment is obsolete: true
(Assignee)

Comment 54

15 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

15 years ago
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.
Attachment #86477 - Attachment is obsolete: true
(Assignee)

Comment 56

15 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

15 years ago
*** Bug 149471 has been marked as a duplicate of this bug. ***
*** Bug 148149 has been marked as a duplicate of this bug. ***

Comment 59

15 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

15 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

15 years ago
this patch passes all of our URL parsing tests.
(Assignee)

Comment 62

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

sr=darin
Attachment #86487 - Flags: superreview+

Comment 63

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

r=dougt
Attachment #86487 - Flags: review+
(Assignee)

Comment 64

15 years ago
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Keywords: mozilla1.0.1
Resolution: --- → FIXED

Comment 65

15 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

15 years ago
first thing tomorrow...sure.

Comment 67

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

Comment 68

15 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

15 years ago
marking VERIFIED per previous two comments.
Status: RESOLVED → VERIFIED

Updated

15 years ago
Attachment #86487 - Flags: approval+

Comment 70

15 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

15 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

15 years ago
Blocks: 143047

Updated

15 years ago
Keywords: adt1.0.1 → adt1.0.1+
(Assignee)

Comment 72

15 years ago
fixed1.0.1
Keywords: mozilla1.0.1+ → fixed1.0.1

Comment 73

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

Comment 74

15 years ago
verified fixd on 0723 brnch.
Keywords: fixed1.0.1 → verified1.0.1
You need to log in before you can comment on or make changes to this bug.