229168 - Save Page (with mms:// embedded media) invokes Windows Media Player

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect, P2)

Description

[Isaac Hwak Han]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20031222
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20031222

.

Reproducible: Always

Steps to Reproduce:
1. Go to the URL
2. Ctrl+S and OK

Actual Results:  
Windows Media Player (WMP) window are invoked.

Expected Results:  
Just save the page without annoying WMP window

This happens only when the saved page has embedded media with src="mms://".

Comment 1

[Bob Hutzel]

I was able to save the page as expected without media player side effects.

WinXP
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20031222

Comment 2

[Hermann Schwab]

wfm Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7a) Gecko/20031222

When you load the page, WMP starts, doesn´t matter, if you press Ctrl+S or not.

marking bug INVALID.

feel free to reopen, if you can tell me how you succeeded to save the page using
CTRL+S without loading (and rendering) it first.

I don´t understand the steps to repeat:

When I load the URL, WMP starts. When I abort WMP, Mozilla has some difficulties
to load other pages, and then an alert comes up that the plugin had done
something, I better should restart Mozilla.

When I use TAB to select the URL: or the real URL in the URL-box, and press
Ctrl-S, I get a box offering me to download show_bug.cgi.html

So I put the cursor over URL:, right-clicked, selected Save Link Target As,
and the file was saved without starting WMP.


the code contains <PARAM NAME=AutoStart VALUE=TRUE>, so if you load the page,
WMP is started automatically, before you can type your Ctrl+S to save the page.
If you want to see the source code of a page, or save it, without rendering it,
prefix the URL with view-source:
view-source:http://www.gamespot.com/live/finder/finder_ad_mi.html


<OBJECT ID="WMPlay" width=320 height=312
classid="CLSID:6BF52A52-394A-11d3-B153-00C04F79FAA6"
   
codebase="http://activex.microsoft.com/activex/controls/mplayer/en/nsmp2inf.cab#Version=5,1,52,701"
    standby="Loading Microsoft Windows Media Player components..."
type="application/x-oleobject">
<PARAM name="URL" value="mms://someLongPath/ad_atart_mios.asf">
<PARAM name=DisplaySize value=0>
<PARAM NAME=ShowControls VALUE=1>
<PARAM NAME=ShowDisplay VALUE=0>
<PARAM NAME=ShowStatusBar VALUE=1>
<PARAM NAME=AutoStart VALUE=TRUE>

<embed width="320" height="312" type="application/x-mplayer2"
pluginspage="http://www.microsoft.com/Windows/Downloads/Contents/Products/MediaPlayer/"
src="mms://someLongPath/ad_atart_mios.asf" Name="GameSpot Live" ShowControls="1"
AutoStart=True ShowDisplay="0" ShowStatusBar="1">
</embed>
</object>

Comment 3

[Isaac Hwak Han]

Reopening by reporter.

It's weird that I can consistently reproduce this bug with a clean profile in
trunk build, mozilla 1.6b, and mozilla 1.5. Let me restate the reproducing steps:

1. Go to the URL (or coming testcase) by clicking
2. The page shows up with embedded media
3. Select File / Save Page As...
4. Make sure that File Type is "Web Page, Complete" (default anyway)
5. Click OK

I have WinXP SP1, and latest WMP (ver 9.00.00.3075).

Saving in view source window or saving by right-clicking at the link (without
opening) works for me too. Also, if the embedded media has src= URL of "http://"
instead of "mms://", it works.

Comment 4

[Isaac Hwak Han]

Attachment: Testcase

Testcase with dummy media

Comment 5

[Boris Zbarsky [:bzbarsky]]

Chances are, the webbrowserpersist code sets a new src on an <object> or
something along those lines, and that triggers a new load to be kicked off.... 
That sounds pretty wrong to me.

Comment 6

[Boris Zbarsky [:bzbarsky]]

The problem is that WebBrowserPersist tries to fetch the mms:// URI, since it's
using a protocol blacklist of things to not fetch, instead of a whitelist for
things to fetch.  It should either use a whitelist or not fetch things for which
the protocol handler is the external protocol handler.  Over to Adam.

Comment 7

[timeless]

Attachment: draft

I found callto: was a better testcase.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 138928 [details] [diff] [review]
draft

Per email with darin, what we want here is a protocol flag that says "never
returns data" and we should not persist anything with that flag....

Comment 9

[Harshal Pradhan]

Attachment: another draft

> Per email with darin, what we want here is a protocol flag 

This adds a flag to nsIProtocolHandler and uses it in nsWebBrowserPersist

Comment 10

[timeless]

Comment on attachment 147236 [details] [diff] [review]
another draft

> NON_PERSISTABLE

I'd rather NOT_PERSISTABLE, although my coworker notes that persistable alone
is bad (it isn't a word).

how about TRANSIENT or perhaps NOT_REQUESTABLE.

Comment 11

[Boris Zbarsky [:bzbarsky]]

We're going to need this flag in more places than just persist.  The real issue
here is whether the protocol returns data or not....

This does leave a problem with javascript:, which returns data but still
shouldn't be persisted.  Similarly with view-source....  But I think that's
something we may want to keep special-casing in the persistence object.  On the
other hand, I see no reason to not persist about: URIs or gopher: URIs.  I
wonder why we don't persist them now; that looks like a bug to me.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 147236 [details] [diff] [review]
another draft

hm, why would finger and datetime not be persistable? and gnome-vfs?

+     * URIs for this protocol dont represent represent objects that

don't

 nsAboutProtocolHandler::GetProtocolFlags(PRUint32 *result)
 {
-    *result = URI_NORELATIVE | URI_NOAUTH;
+    *result = URI_NORELATIVE | URI_NOAUTH | NON_PERSISTABLE;

um, why are these not persistable?

and gopher?

personally I think you're adding this to way too many protocols

Comment 13

[Darin Fisher]

Comment on attachment 147236 [details] [diff] [review]
another draft

I agree with biesi's comments.	I know that nsWebBrowserPersist blacklisted
gopher and finger, but I can't think of a good reason for that.

Also, I'm not sure NON_PERSISTABLE is the right choice of names for this flag. 
I would have used a name that better reflects the fact that it does not
generate content... like: URI_NOCONTENT

Comment 14

[Harshal Pradhan]

OK. I'll change the name to URI_NOCONTENT and not include things like about,
datetime, finger and gopher. Those had bothered me too, but I was trying to be
consitent with what the current code does. The only place I guess I mistakenlly
added the restriction was the gnomevfs handler. Having looked at the code, I now
realize that we have a pref and restrict its usage to trusted, "data-only"
protocols. So, using the flag there was totally wrong.

So, like bz said, we let the flag mean "this protocol returns no data", and keep
special-casing javascript and viewsource in the persist object. Right? 

Comment 15

[Darin Fisher]

Seems reasonable to me.

Comment 16

[Stefan Borggraefe]

I think news: shouldn't have the URI_NOCONTENT flag, too. At least going to a
news: URL like

  news://news.mozilla.org:119/36718AB4.4134F58C%40netscape.com

and doing Save Page As... in Navigator works just fine currently.

Comment 17

[rossi]

is there any progress in this bug? i'm very concerned about dependent bug
181860. i visited a site with firefox 1.0 some days ago and it made me pop up
mail windows until i could kill it through <img src=mailto:> tags. 

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

should this maybe be a channel flag in some way too? that may be easier for
callers to check - they can create the channel as usual, and if they don't want
external content, they can check the flag and return.

or, maybe, can we do something entirely different, and assume most callers do
not want external content, and have asyncOpen fail unless a special load flag is
set?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Hmm... The second suggestion in comment 18 has some merit if we'd done it that
way to start with, but I think that would be a too-big nsIChannel API change....

Flags on the channel is doable, but again has api issues.

I agree that most callers don't want no-data protocols as things stand; the one
exception I'm aware of is a load in a docshell.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

I'll look at this.

Comment 21

[Jay Patel [:jay]]

Not going to make 1.8.0.5, we need some traction and a possible patch before considering this... so if a patch shows up, please nominate for the next release.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Sorry, I got swamped with other stuff to work on, so I won't, in fact, get a chance to look at this soon.

Comment 23

[Darin Fisher]

Not going to block 1.8.1 for this, but we'd be happy to consider a patch that's baked on the trunk.

Comment 24

[Ryan Jones-Ward [:sciguyryan]]

Assigning this to myself and I'll to take a look as soon as I can (tomorrow I hope). If anyone else if working on this or post a fix before me please re-assign it to yourself. Thanks!

Comment 25

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1

Patch v1

Comment 26

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1 (again)

Patch v1 without the spelling error.

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 256301 [details] [diff] [review]
Patch v1 (again)

why are about, data, view-source, ldap, javascript and news not persistable?

I think addbook should not have this flag either, not quite sure about that one though...

also, it seems to me like you're breaking compatibility for chatzilla here - don't you need something like what it does for URI_DANGEROUS_TO_LOAD?

Comment 28

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #27)
> (From update of attachment 256301 [details] [diff] [review])
> why are about, data, view-source, ldap, javascript and news not persistable?
>

The data protocol was recently added too the non-persistable list to fix bug 155109 (see justification there). Javascript suffers the same as I understand it, we don't really need to save in-line Javascript to a file.

> I think addbook should not have this flag either, not quite sure about that one
> though...

I've removed it, it can always be re-added if needed (any news experts about here to answer this one?)

> 
> also, it seems to me like you're breaking compatibility for chatzilla here -
> don't you need something like what it does for URI_DANGEROUS_TO_LOAD?
> 

Fixed, good catch.

Comment 29

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1.1

Patch v1.1

Comment 30

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v2

Patch v2 per the discussion on IRC.

Comment 31

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v2.1

Patch v2.1

Added the "URI_" prefix to the flag in compliance with the other flags of this type.

Comment 32

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 256521 [details] [diff] [review]
Patch v2.1

+    NS_ENSURE_SUCCESS(rv, rv);

I'm not sure this is the best way to handle this. I'd do:
  if (NS_FAILED(rv))
    doNotPersistURI = PR_FALSE;

I'd also move the variable declaration to right before the NS_URIChainHasFlags call and wouldn't initialize it.

silver, this patch touches venkman/chatzilla, so you might want to look at this.

Comment 33

[James Ross]

Comment on attachment 256521 [details] [diff] [review]
Patch v2.1

r=silver on the Venkman and ChatZilla parts.

However, I should note that a good number of x-jsd URLs do produce content that can be saved fine; I'm not convinced this flag should be applied to that protocol.

Comment 34

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #33)
> (From update of attachment 256521 [details] [diff] [review])
> r=silver on the Venkman and ChatZilla parts.
> 
> However, I should note that a good number of x-jsd URLs do produce content that
> can be saved fine; I'm not convinced this flag should be applied to that
> protocol.
> 

If you think most can be saves safely then I am fine with removing the Venkman. Biesi, any thoughts too?

Comment 35

[Christian :Biesinger (don't email me, ping me on IRC)]

sounds good

Comment 36

[Ryan Jones-Ward [:sciguyryan]]

Attachment: For checkin

Patch for checkin, updated to comments.

Comment 37

[:Gijs (he/him)]

Checked in.