239472 - Page Info -> media -> Save As... doesn't work

Status: VERIFIED FIXED

(SeaMonkey :: Page Info, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

[Alfonso Martinez]

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

[HARUNAGA Hirotoshi]

I filed the issue only on Mac as Bug 239535.

Comment 3

[PikeUK]

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

[OstGote!]

Still there in trunk build 2004042608 on WinNT4.

Comment 5

[José Jeria]

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

Comment 6

[Frank Burleigh]

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

[OstGote!]

(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

[Anders Pedersen]

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

[PikeUK]

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

Comment 10

[Tom Leski]

It looks like the bug has been resolved in the latest nightly - works fine for me.

Comment 11

[OstGote!]

Bug is still there in Mozilla trunk build 2004052709 on WinNT4.

Comment 12

[OstGote!]

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

[Florian Quèze [:florian]]

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

[Florian Quèze [:florian]]

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

[Giacomo Magnini]

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

[HARUNAGA Hirotoshi]

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

Comment 17

[OstGote!]

(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

[OstGote!]

(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

[Daniel Brooks [:db48x]]

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

[Daniel Brooks [:db48x]]

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

[Florian Quèze [:florian]]

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

[Florian Quèze [:florian]]

Comment on attachment 157758 [details] [diff] [review]
a better patch

requesting review

Comment 23

[Florian Quèze [:florian]]

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

[Daniel Brooks [:db48x]]

Comment on attachment 157799 [details] [diff] [review]
patch for Firefox

r=db48x

Comment 25

[Ryan Polk (Quark)]

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

[Jim Ginn]

(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

[Ryan Polk (Quark)]

Comment on attachment 157799 [details] [diff] [review]
patch for Firefox

Alright, so this patch should go in and that one should not.

Comment 28

[Florian Quèze [:florian]]

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

[Daniel Brooks [:db48x]]

Comment on attachment 157758 [details] [diff] [review]
a better patch

r=db48x on this part too

Comment 30

[chris hofmann]

ben, if this looks like a good patch, lets approve and get it in.

Comment 31

[Asa Dotzler [:asa]]

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

[jag (Peter Annema)]

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

[jag (Peter Annema)]

Btw, Daniel, with bug 72522 and bug 72524 fixed, should we kill getAbsoluteURL()?

Comment 34

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 157799 [details] [diff] [review]
patch for Firefox

sr+a=[e-mail]

Comment 35

[Florian Quèze [:florian]]

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

[Florian Quèze [:florian]]

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

[Florian Quèze [:florian]]

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

[Asa Dotzler [:asa]]

we'd consider the fully reviewed patch but this isn't going to block the release. 

Comment 39

[Asa Dotzler [:asa]]

Comment on attachment 161256 [details] [diff] [review]
patch

please don't request approval until a patch is fully reviewed. thanks.

Comment 40

[jag (Peter Annema)]

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

[Florian Quèze [:florian]]

(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

[Florian Quèze [:florian]]

Attachment: updated patch

Comment 43

[jag (Peter Annema)]

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

[Daniel Brooks [:db48x]]

Comment on attachment 162252 [details] [diff] [review]
updated patch

r=db48x

Comment 45

[Asa Dotzler [:asa]]

Comment on attachment 162252 [details] [diff] [review]
updated patch

a=asa

Comment 46

[Daniel Brooks [:db48x]]

checked in

Comment 47

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

interesting... bug 258185's patch is somewhat similar

Comment 48

[Tracy Walker [:tracy]]

verified with windows firefox branch build 2004102607

Comment 49

[[e-mail]]

And my comment there about aReferrer applies here too :-/

Comment 50

[Hiro]

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

[OstGote!]

(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

[mcfang]

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

[Daniel Brooks [:db48x]]

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

[Daniel Brooks [:db48x]]

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

Comment 55

[Mike Kaply [:mkaply]]

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

[Daniel Brooks [:db48x]]

checked attachment 161256 [details] [diff] [review] into the MOZILLA_1_7_BRANCH

Comment 57

[Florian Quèze [:florian]]

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