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

RESOLVED FIXED in mozilla1.9alpha3

Status

defect
P2
normal
RESOLVED FIXED
16 years ago
5 months ago

People

(Reporter: isaachh, Assigned: sciguyryan)

Tracking

(Blocks 1 bug, {top100})

Trunk
mozilla1.9alpha3
Dependency tree / graph
Bug Flags:
blocking1.9 +
blocking1.8.1 -
blocking1.8.0.5 -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 attachments, 7 obsolete attachments)

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://".
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
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>
Status: UNCONFIRMED → RESOLVED
Closed: 16 years ago
Resolution: --- → INVALID
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.
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
Posted file Testcase
Testcase with dummy media
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.
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.
Assignee: file-handling → adamlock
Status: UNCONFIRMED → NEW
Component: File Handling → Embedding: APIs
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Posted patch draft (obsolete) — Splinter Review
I found callto: was a better testcase.
Attachment #138928 - Flags: superreview?(bz-vacation)
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....
Attachment #138928 - Flags: superreview?(bz-vacation) → superreview-
Keywords: top100
Posted patch another draft (obsolete) — Splinter Review
> Per email with darin, what we want here is a protocol flag 

This adds a flag to nsIProtocolHandler and uses it in nsWebBrowserPersist
Attachment #147236 - Flags: review?(darin)
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.
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 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 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
Attachment #147236 - Flags: review?(darin) → review-
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? 
Seems reasonable to me.
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.
No longer blocks: 167475
Depends on: 167475
Blocks: 167475
No longer depends on: 167475
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. 
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?
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.
I'll look at this.
Assignee: adamlock → mrbkap
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Flags: blocking1.8.0.5?
Whiteboard: [sg:want]
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.
Flags: blocking1.8.0.5? → blocking1.8.0.5-
Sorry, I got swamped with other stuff to work on, so I won't, in fact, get a chance to look at this soon.
Assignee: mrbkap → nobody
QA Contact: ian → apis
Not going to block 1.8.1 for this, but we'd be happy to consider a patch that's baked on the trunk.
Flags: blocking1.8.1? → blocking1.8.1-
Flags: blocking1.9a1? → blocking1.9+
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!
Assignee: nobody → sciguyryan
Status: NEW → ASSIGNED
Target Milestone: mozilla1.9alpha1 → mozilla1.9alpha3
Posted patch Patch v1 (obsolete) — Splinter Review
Patch v1
Attachment #138928 - Attachment is obsolete: true
Attachment #147236 - Attachment is obsolete: true
Attachment #256300 - Flags: review?(cbiesinger)
Posted patch Patch v1 (again) (obsolete) — Splinter Review
Patch v1 without the spelling error.
Attachment #256300 - Attachment is obsolete: true
Attachment #256301 - Flags: review?(cbiesinger)
Attachment #256300 - Flags: review?(cbiesinger)
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?
(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.
Posted patch Patch v1.1 (obsolete) — Splinter Review
Patch v1.1
Attachment #256301 - Attachment is obsolete: true
Attachment #256483 - Flags: review?(cbiesinger)
Attachment #256301 - Flags: review?(cbiesinger)
Attachment #256483 - Flags: review?(cbiesinger)
Posted patch Patch v2 (obsolete) — Splinter Review
Patch v2 per the discussion on IRC.
Attachment #256483 - Attachment is obsolete: true
Attachment #256520 - Flags: review?(cbiesinger)
Posted patch Patch v2.1 (obsolete) — Splinter Review
Patch v2.1

Added the "URI_" prefix to the flag in compliance with the other flags of this type.
Attachment #256520 - Attachment is obsolete: true
Attachment #256521 - Flags: review?(cbiesinger)
Attachment #256520 - Flags: review?(cbiesinger)
Attachment #256521 - Flags: superreview?(cbiesinger)
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.
Attachment #256521 - Flags: superreview?(cbiesinger)
Attachment #256521 - Flags: superreview+
Attachment #256521 - Flags: review?(silver)
Attachment #256521 - Flags: review?(cbiesinger)
Attachment #256521 - Flags: review+
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.
Attachment #256521 - Flags: review?(silver) → review+
(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?
Posted patch For checkinSplinter Review
Patch for checkin, updated to comments.
Attachment #256521 - Attachment is obsolete: true
Whiteboard: [sg:want] → [sg:want] [checkin needed]
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago13 years ago
Resolution: --- → FIXED
Whiteboard: [sg:want] [checkin needed] → [sg:want]
Flags: wanted1.8.1.x?
Flags: wanted1.8.1.x?
Whiteboard: [sg:want]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.