Last Comment Bug 239472 - Page Info -> media -> Save As... doesn't work
: Page Info -> media -> Save As... doesn't work
Status: VERIFIED FIXED
[have patch] ready to land
: fixed-aviary1.0, fixed1.7.6, regression
Product: SeaMonkey
Classification: Client Software
Component: Page Info (show other bugs)
: Trunk
: x86 All
-- major with 2 votes (vote)
: ---
Assigned To: Florian Quèze [:florian] [:flo] (PTO until February 27)
:
:
Mentors:
: 242032 244623 254984 275487 285609 (view as bug list)
Depends on:
Blocks: 241865 258185
  Show dependency treegraph
 
Reported: 2004-04-02 16:47 PST by Masayuki Nakano [:masayuki]
Modified: 2014-06-13 08:10 PDT (History)
29 users (show)
asa: blocking‑aviary1.0-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (1.89 KB, patch)
2004-08-01 16:59 PDT, Florian Quèze [:florian] [:flo] (PTO until February 27)
no flags Details | Diff | Splinter Review
a better patch (4.38 KB, patch)
2004-09-02 16:56 PDT, Florian Quèze [:florian] [:flo] (PTO until February 27)
db48x: review+
Details | Diff | Splinter Review
patch for Firefox (3.17 KB, patch)
2004-09-03 05:52 PDT, Florian Quèze [:florian] [:flo] (PTO until February 27)
db48x: review+
bugs: superreview+
bugs: approval‑aviary+
Details | Diff | Splinter Review
patch (7.47 KB, patch)
2004-10-06 04:53 PDT, Florian Quèze [:florian] [:flo] (PTO until February 27)
jag-mozilla: superreview+
Details | Diff | Splinter Review
updated patch (7.07 KB, patch)
2004-10-15 16:35 PDT, Florian Quèze [:florian] [:flo] (PTO until February 27)
db48x: review+
jag-mozilla: superreview+
asa: approval‑aviary+
mozilla: approval1.7.6+
Details | Diff | Splinter Review

Description User image Masayuki Nakano [:masayuki] 2004-04-02 16:47:13 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040401
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040401

I cannot download from "Save As..." button that is on "media" tab on Page Info
Window.

It can be reproduced with WinXP and Linux.
On Mac, media tab is not completed to initialize.

Reproducible: Always
Steps to Reproduce:
1. Select image or other object, but it is not type = background(See bug 178469).
2. Push "Save As..." button.
3. Choose file name.

Actual Results:  
It is added to download manager or opened progless window.
But It is not started to download.

Expected Results:  
It should be started to download, and should be completed.
Comment 1 User image Alfonso Martinez 2004-04-03 03:36:54 PST
Confirming with wXP 2004040110.

The Javascript console shows the following error.

Error: [Exception... "Component returned failure code: 0x80570009
(NS_ERROR_XPC_BAD_CONVERT_JS) [nsIXPCComponents.lookupMethod]"  nsresult:
"0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)"  location: "JS frame ::
chrome://communicator/content/contentAreaUtils.js :: getContentFrameURI :: line
72"  data: no]
Source File: chrome://communicator/content/contentAreaUtils.js
Line: 72

Adding CC: Gilles Durys as he created the patch for bug 228218 and might want to
take a look at this.
Comment 2 User image HARUNAGA Hirotoshi 2004-04-03 12:10:11 PST
I filed the issue only on Mac as Bug 239535.
Comment 3 User image PikeUK 2004-04-08 02:47:32 PDT
In Firefox this regressed between 2004-03-03 and 2004-03-04, this points to the
checkin from Bug 236194 (which was a Firefox port of the patch from Bug 196871).
Removing that patch fixes this problem.

I assume that Seamonkey regressed on the day that Bug 196871 was checked in
though I haven't actually confirmed that.

One obvious fix is just to move the start of the try statement in getReferrer()
(from contentAreaUtils.js) up a bit so that the failing line is caught by the
catch and the function just returns null (or would it be better to prevent
getReferrer from being called in the first place?).

http://lxr.mozilla.org/mozilla/source/browser/base/content/contentAreaUtils.js#118
http://lxr.mozilla.org/mozilla/source/xpfe/communicator/resources/content/contentAreaUtils.js#75

Should I file a separate Firefox bug (since this code is branched) or will both
apps be handled here?
Comment 4 User image OstGote! 2004-04-27 23:48:18 PDT
Still there in trunk build 2004042608 on WinNT4.
Comment 5 User image José Jeria 2004-04-29 00:19:41 PDT
*** Bug 242032 has been marked as a duplicate of this bug. ***
Comment 6 User image Frank Burleigh 2004-05-01 12:18:56 PDT
I see this also on XP with the latest Moz nightly.  Images characterized on the
media tab as "image" can download, but those known as "background" don't save:
the Save As button does nothing.  Seems like this has now been confirmed several
times eh? ;-)
Comment 7 User image OstGote! 2004-05-03 01:22:55 PDT
(In reply to comment #6)
> I see this also on XP with the latest Moz nightly.  Images characterized on the
> media tab as "image" can download, but those known as "background" don't save:
> the Save As button does nothing.

That is Bug 178469.

Comment 8 User image Anders Pedersen 2004-05-14 15:16:26 PDT
Confirmed.

I get the images in the download manager but with the state "Starting..." all
the time and not being downloaded.

- Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a) Gecko/20040514
Firefox/0.8.0+
- Microsoft Windows 2000 Pro 5.00.2195 SP4
Comment 9 User image PikeUK 2004-05-25 07:25:37 PDT
*** Bug 244623 has been marked as a duplicate of this bug. ***
Comment 10 User image Tom Leski 2004-05-25 16:43:50 PDT
It looks like the bug has been resolved in the latest nightly - works fine for me.
Comment 11 User image OstGote! 2004-05-28 02:41:12 PDT
Bug is still there in Mozilla trunk build 2004052709 on WinNT4.
Comment 12 User image OstGote! 2004-07-29 23:43:21 PDT
Mozilla 2004072808 WinNT4.0
Now I get the following error in JS console if I try a Save As in the media tab
for a normal image (appears after selecting save in the save as dialog):

Error: XPCNativeWrapper is not defined
Source File: chrome://communicator/content/contentAreaUtils.js
Line: 48
Comment 13 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2004-08-01 16:59:55 PDT
Created attachment 154948 [details] [diff] [review]
patch

As proposed in comment #3, start of the try statement in getReferrer()
(from contentAreaUtils.js) moved up a bit so that the failing line is caught by
the catch and the function just returns null.

<script type="application/x-javascript"
src="chrome://communicator/content/XPCNativeWrapper.js"/>
added to pageInfo.xul to fix error in comment #12
Comment 14 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2004-08-01 17:13:07 PDT
Comment on attachment 154948 [details] [diff] [review]
patch

The error described in comment #12 seems to occur since the check in of patchs
from bug 220215.

It's fixed the same way in bug 248964.
Comment 15 User image Giacomo Magnini 2004-08-07 04:13:23 PDT
I think the same problem caused bug 248964, and I have the same symptoms even
for downloading some files (ie., if I open directly an URL ending with a
filename to download in a new browser window, without navigating the site.
Comment 16 User image HARUNAGA Hirotoshi 2004-08-10 21:41:45 PDT
*** Bug 254984 has been marked as a duplicate of this bug. ***
Comment 17 User image OstGote! 2004-08-11 02:35:28 PDT
(In reply to comment #14)
> It's fixed the same way in bug 248964.

Bug 248964 is now fixed and it works with Mozilla 2004081008 on WinNT4. But Save
As for Page Info/Media does still not work. But the error has changed.

Now I get the following error in JS console if I try a Save As in the media tab
for a normal image (appears after selecting save in the save as dialog). Note:
No download progress dialog appears (or it is too fast).

Error: [Exception... "Component returned failure code: 0x80040154
(NS_ERROR_FACTORY_NOT_REGISTERED) [nsIDownload.init]"  nsresult: "0x80040154
(NS_ERROR_FACTORY_NOT_REGISTERED)"  location: "JS frame ::
chrome://communicator/content/contentAreaUtils.js :: foundHeaderInfo :: line
404"  data: no]
Source File: chrome://communicator/content/contentAreaUtils.js
Line: 404
Comment 18 User image OstGote! 2004-08-12 03:24:23 PDT
(In reply to comment #17)
> But the error has changed.

Build 2004081109 on WinNT4 changed back again, amazing. Now it is the same error
like in comment 12. The Saving dialog pops up but never finishes or saves something.
Comment 19 User image Daniel Brooks [:db48x] 2004-08-12 16:31:59 PDT
I need to trace through all of this, since I don't really know how it works. I'm
guessing, but it's probably trying to use the page info window as the window to
get the referring url from, which is wrong. Or something. At any rate, this will
have to wait since I'm stuck at my parents place and my Dad just volunteered me
to help put plywood over the windows. :P I hate hurricanes.
Comment 20 User image Daniel Brooks [:db48x] 2004-08-12 16:40:08 PDT
Oh, and it has to send the correct referrer, because there's a certain commonly
accessed class of websites (porn) that check the referrer you send to see if you
are allowed to view the images that page contains, even though this isn't a very
robust authorization method.
Comment 21 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2004-09-02 16:56:36 PDT
Created attachment 157758 [details] [diff] [review]
a better patch

With this patch, there isn't javascript error anymore, it sends the correct
referrer and it also saves background images correctly.
Comment 22 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2004-09-02 17:01:07 PDT
Comment on attachment 157758 [details] [diff] [review]
a better patch

requesting review
Comment 23 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2004-09-03 05:52:05 PDT
Created attachment 157799 [details] [diff] [review]
patch for Firefox

A similar patch for Firefox. With this patch, Firefox sends referrer when
saving from page info. It also saves correctly background images (see bug
178469) and images embedded with the OBJECT tag (see bug 235447).

Firefox wasn't affected by javascript errors described in comment 1 and in
comment 12.
Comment 24 User image Daniel Brooks [:db48x] 2004-09-27 06:33:33 PDT
Comment on attachment 157799 [details] [diff] [review]
patch for Firefox

r=db48x
Comment 25 User image Ryan Polk (Quark) 2004-09-28 05:52:12 PDT
What's the difference in the patch here and the patch for bug 178469?
Looks like this one includes that patch and then some extra changes.
Comment 26 User image Jim Ginn 2004-09-28 06:40:59 PDT
(In reply to comment #25)
> What's the difference in the patch here and the patch for bug 178469?
> Looks like this one includes that patch and then some extra changes.

As Florian stated this fixes bug 178469 and images embedded with the OBJECT tag
(bug 235447). Since this is a more complete fix we probably want this over the
patch in 178469.
Comment 27 User image Ryan Polk (Quark) 2004-09-28 07:06:09 PDT
Comment on attachment 157799 [details] [diff] [review]
patch for Firefox

Alright, so this patch should go in and that one should not.
Comment 28 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2004-09-28 08:40:11 PDT
Comment on attachment 157758 [details] [diff] [review]
a better patch

attachment 157758 [details] [diff] [review] is for seamonkey and 157799 is for firefox.
Comment 29 User image Daniel Brooks [:db48x] 2004-09-28 13:28:09 PDT
Comment on attachment 157758 [details] [diff] [review]
a better patch

r=db48x on this part too
Comment 30 User image chris hofmann 2004-09-28 19:31:40 PDT
ben, if this looks like a good patch, lets approve and get it in.
Comment 31 User image Asa Dotzler [:asa] 2004-10-03 11:30:22 PDT
Comment on attachment 157799 [details] [diff] [review]
patch for Firefox

if this is still pending review, please don't request approval for landing.
thanks.
Comment 32 User image jag (Peter Annema) 2004-10-05 02:03:32 PDT
Comment on attachment 157758 [details] [diff] [review]
a better patch

Since use of referrer depends on url not being null (and if url != null, then
item != null holds), either move the var referrer inside the |if (url)| without
the |if (item)| or apply the "avoid one time vars" rule and inline it, so:

if (url)
  saveURL(url, null, 'SaveImageTitle', false, makeURL(item.baseURI));

(yes, I removed that silly space before ')', per the style guide)

-    var referer = getReferrer(document);
+    var referer = aData.referrer || getReferrer(document);


So why is that using |document| instead of |aData.document|? Would that solve
our problem instead of passing in the referrer explicitely?
Comment 33 User image jag (Peter Annema) 2004-10-05 02:08:27 PDT
Btw, Daniel, with bug 72522 and bug 72524 fixed, should we kill getAbsoluteURL()?
Comment 34 User image Ben Goodger (use ben at mozilla dot org for email) 2004-10-05 20:31:10 PDT
Comment on attachment 157799 [details] [diff] [review]
patch for Firefox

sr+a=ben@mozilla.org
Comment 35 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2004-10-06 04:53:23 PDT
Created attachment 161256 [details] [diff] [review]
patch

(In reply to comment #32)
> (From update of attachment 157758 [details] [diff] [review])
> Since use of referrer depends on url not being null (and if url != null, then

> item != null holds), either move the var referrer inside the |if (url)|
without
> the |if (item)| or apply the "avoid one time vars" rule and inline it, so:
> 
Yes, item can't be null if url isn't.

> if (url)
>   saveURL(url, null, 'SaveImageTitle', false, makeURL(item.baseURI));
> 
> (yes, I removed that silly space before ')', per the style guide)
I prefer this.
 
> -    var referer = getReferrer(document);
> +    var referer = aData.referrer || getReferrer(document);
> 
> 
> So why is that using |document| instead of |aData.document|? Would that solve

> our problem instead of passing in the referrer explicitely?
> 

saveInternal is sometimes called with null for aDocument, for example by
saveDocument or SaveURL (SaveURL isn't only called by page info).
So we can't use getReferrer(aData.document) because when aData.document is
null, getReferrer yields the error "TypeError: doc has no properties".
Moreover, passing in the referrer explicitely is faster: getReferrer calls
getContentFrameURI which calls isContentFrame and XPCNativeWrapper.

The new version of the patch includes the fix for both Seamonkey and Firefox in
one patch.
It uses "saveURL(url, null, 'SaveImageTitle', false, makeURL(item.baseURI));".
Comment 36 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2004-10-06 05:01:35 PDT
Comment on attachment 161256 [details] [diff] [review]
patch

Ben, if the previous patch isn't checked in Aviary yet, i prefer this new one,
it is shorter. The only change is
  if (url)
    saveURL(url, null, 'SaveImageTitle', false, makeURL(item.baseURI));

instead of 

  if (item)
    var referrer = makeURL(item.baseURI);

  if (url)
    saveURL(url, null, 'SaveImageTitle', false, false, referrer);
Comment 37 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2004-10-06 05:17:03 PDT
my comment 36 is a litle confusing.

For Firefox, the change between attachment 157799 [details] [diff] [review] and the new patch is:
  if (url)
    saveURL(url, null, 'SaveImageTitle', false, false, makeURL(item.baseURI));

instead of 

  if (item)
    var referrer = makeURL(item.baseURI);

  if (url)
    saveURL(url, null, 'SaveImageTitle', false, false, referrer);

and for Seamonkey it is:
  if (url)
    saveURL(url, null, 'SaveImageTitle', false, makeURL(item.baseURI));

instead of 

  if (item)
    var referrer = makeURL(item.baseURI);

  if (url)
    saveURL(url, null, 'SaveImageTitle', false, referrer );

In comment 36, ", false" was lacking.
sorry for bug spam
Comment 38 User image Asa Dotzler [:asa] 2004-10-06 16:39:04 PDT
we'd consider the fully reviewed patch but this isn't going to block the release. 
Comment 39 User image Asa Dotzler [:asa] 2004-10-11 11:42:49 PDT
Comment on attachment 161256 [details] [diff] [review]
patch

please don't request approval until a patch is fully reviewed. thanks.
Comment 40 User image jag (Peter Annema) 2004-10-11 14:55:41 PDT
Comment on attachment 161256 [details] [diff] [review]
patch

Florian: a recent checkin to contentAreaUtils.js makes this patch no longer
apply. I don't think it's hard to update.

Otherwise it looks good. I buy your argument on passing in aReferrer
explicitely. Are there other places that could do that, taking the guess-work
out of it?

Also, if aData.document can be null, how about var referrer = aData.referrer ||
getReferrer(aData.document || document)?

Though it seems in the new code |aData| is gone. Ah, but there's |aDocument|
instead. Anyway, just some ponderings.

sr=jag, but marking obsolete to indicate it needs updating.
Comment 41 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2004-10-15 15:36:00 PDT
(In reply to comment #40)
 
> Also, if aData.document can be null, how about var referrer = aData.referrer ||
> getReferrer(aData.document || document)?
>

0001: aDocument  (it's the same type for aData.document)
$[0] = [HTMLDocument] [class: HTMLDocument] {2}
0001: document
$[1] = [XULDocument] [class: XULDocument] {1}

(i pasted this from Venkman)
getReferrer expects a parameter of type "XULDocument", so we can't use
getReferrer(aData.document || document).

I've tried to use:
var referrer = aDocument ? makeURI(aDocument.URL) : getReferrer(document)

We can get an [HTMLdocument] value from page info with:
imageView.data[tree.currentIndex][3].ownerDocument

But passing this value to internalSave as aDocument doesn't work because
internalSave uses this value to find the content type of the file to save, it
detects text/html type and then fails in "getPostData()" :
"Error ``getWebNavigation is not defined'' [xs] in file
``chrome://communicator/content/contentAreaUtils.js'', line 585, character 0.".

 
So I think i'll have to pass the referrer explicitely in the updated patch, like
i did in previous patch.
Comment 42 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2004-10-15 16:35:00 PDT
Created attachment 162252 [details] [diff] [review]
updated patch
Comment 43 User image jag (Peter Annema) 2004-10-15 17:38:54 PDT
Comment on attachment 162252 [details] [diff] [review]
updated patch

Yeah, like I said, passing the referrer in explicitely seems the right way to
go.

I guess what bugs me is that saveInternal assumes that |document| is the
document containing the item you want to save. If that could be made more
explicit, e.g. |getReferrer(aContainingDocument)|, then that code would work.
Alternatively, the only place that refers to |document| is that getReferrer
call, so if that can be avoided by passing in the refferer explicitely in all
calls, life will be good too. Perhaps in another bug report though.

sr=jag
Comment 44 User image Daniel Brooks [:db48x] 2004-10-24 06:02:06 PDT
Comment on attachment 162252 [details] [diff] [review]
updated patch

r=db48x
Comment 45 User image Asa Dotzler [:asa] 2004-10-25 19:50:06 PDT
Comment on attachment 162252 [details] [diff] [review]
updated patch

a=asa
Comment 46 User image Daniel Brooks [:db48x] 2004-10-26 00:10:14 PDT
checked in
Comment 47 User image Christian :Biesinger (don't email me, ping me on IRC) 2004-10-26 05:36:32 PDT
interesting... bug 258185's patch is somewhat similar
Comment 48 User image Tracy Walker [:tracy] 2004-10-26 09:09:03 PDT
verified with windows firefox branch build 2004102607
Comment 49 User image neil@parkwaycc.co.uk 2004-10-26 09:17:37 PDT
And my comment there about aReferrer applies here too :-/
Comment 50 User image Hiro 2004-12-22 12:40:43 PST
It still reproduces by Mozilla1.7.5.
Is correction checkin by MOZILLA_1_7_BRANCH?

Mac OS X 1.7.5
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.5) Gecko/20041217
Comment 51 User image OstGote! 2005-01-04 02:44:21 PST
(In reply to comment #50)
> It still reproduces by Mozilla1.7.5.
see Bug 275487 for confirming that

> Is correction checkin by MOZILLA_1_7_BRANCH?
Looks like it was not checked in.
Comment 52 User image mcfang 2005-01-04 21:43:48 PST
I have tested and this bug is still active in Moz 1.7.5, 1.7.3, 1.7.2 (and
probably more older versions).

Luckilly the fix from Comment 3 still works in these versions (moving the TRY
statement in the contentAreaUtils.js file).
Comment 53 User image Daniel Brooks [:db48x] 2005-01-15 02:25:39 PST
Comment on attachment 162252 [details] [diff] [review]
updated patch

requesting a= for 1.7 branch. not security related or anything special like
that, just really annoying to keep fielding bugs about this.
Comment 54 User image Daniel Brooks [:db48x] 2005-01-15 02:27:32 PST
*** Bug 275487 has been marked as a duplicate of this bug. ***
Comment 55 User image Mike Kaply [:mkaply] 2005-01-17 06:17:40 PST
Comment on attachment 162252 [details] [diff] [review]
updated patch

a=mkaply for 1.7.6 as this is a firefox/mozilla compat issue.
Comment 56 User image Daniel Brooks [:db48x] 2005-02-04 11:33:25 PST
checked attachment 161256 [details] [diff] [review] into the MOZILLA_1_7_BRANCH
Comment 57 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2005-03-10 12:38:43 PST
*** Bug 285609 has been marked as a duplicate of this bug. ***

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