Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED

Status

SeaMonkey
Page Info
--
major
VERIFIED FIXED
14 years ago
3 years ago

People

(Reporter: masayuki, Assigned: florian)

Tracking

({fixed-aviary1.0, fixed1.7.6, regression})

Trunk
x86
All
fixed-aviary1.0, fixed1.7.6, regression
Dependency tree / graph
Bug Flags:
blocking-aviary1.0 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [have patch] ready to land)

Attachments

(1 attachment, 4 obsolete attachments)

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

14 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

14 years ago
I filed the issue only on Mac as Bug 239535.

Comment 3

14 years ago
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?

Updated

13 years ago
Blocks: 241865

Comment 4

13 years ago
Still there in trunk build 2004042608 on WinNT4.

Updated

13 years ago
Keywords: regression

Comment 5

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

Comment 6

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

13 years ago
(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

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

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

Comment 10

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

Comment 11

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

Comment 12

13 years ago
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
(Assignee)

Comment 13

13 years ago
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
(Assignee)

Comment 14

13 years ago
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.
Attachment #154948 - Flags: review?

Comment 15

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

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

Comment 17

13 years ago
(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

13 years ago
(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.
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.
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.
(Assignee)

Comment 21

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

Comment 22

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

requesting review
Attachment #157758 - Flags: review?(db48x)

Updated

13 years ago
Flags: blocking-aviary1.0?
(Assignee)

Comment 23

13 years ago
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.

Updated

13 years ago
Attachment #157758 - Flags: review?(db48x)
Comment on attachment 157799 [details] [diff] [review]
patch for Firefox

r=db48x
Attachment #157799 - Flags: review+

Updated

13 years ago
Attachment #154948 - Flags: review?

Comment 25

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

13 years ago
(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

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

Alright, so this patch should go in and that one should not.
Attachment #157799 - Flags: approval-aviary?
(Assignee)

Comment 28

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

attachment 157758 [details] [diff] [review] is for seamonkey and 157799 is for firefox.
Attachment #157758 - Flags: review?(db48x)
Comment on attachment 157758 [details] [diff] [review]
a better patch

r=db48x on this part too
Attachment #157758 - Flags: review?(db48x) → review+

Comment 30

13 years ago
ben, if this looks like a good patch, lets approve and get it in.
Flags: blocking-aviary1.0? → blocking-aviary1.0+
(Assignee)

Updated

13 years ago
Attachment #157799 - Flags: superreview?(jag)
(Assignee)

Updated

13 years ago
Attachment #157758 - Flags: superreview?(jag)

Comment 31

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

if this is still pending review, please don't request approval for landing.
thanks.
Attachment #157799 - Flags: approval-aviary?

Comment 32

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

13 years ago
Btw, Daniel, with bug 72522 and bug 72524 fixed, should we kill getAbsoluteURL()?
Comment on attachment 157799 [details] [diff] [review]
patch for Firefox

sr+a=ben@mozilla.org
Attachment #157799 - Flags: superreview?(jag)
Attachment #157799 - Flags: superreview+
Attachment #157799 - Flags: approval-aviary+
(Assignee)

Comment 35

13 years ago
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));".
(Assignee)

Comment 36

13 years ago
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);
Attachment #161256 - Flags: superreview?(jag)
Attachment #161256 - Flags: review?(db48x)
Attachment #161256 - Flags: approval-aviary?
(Assignee)

Comment 37

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

Updated

13 years ago
Whiteboard: [have patch] ready to land

Comment 38

13 years ago
we'd consider the fully reviewed patch but this isn't going to block the release. 
Flags: blocking-aviary1.0+ → blocking-aviary1.0-

Comment 39

13 years ago
Comment on attachment 161256 [details] [diff] [review]
patch

please don't request approval until a patch is fully reviewed. thanks.
Attachment #161256 - Flags: approval-aviary?

Comment 40

13 years ago
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.
Attachment #161256 - Attachment is obsolete: true
Attachment #161256 - Flags: superreview?(jag) → superreview+
(Assignee)

Comment 41

13 years ago
(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.
(Assignee)

Comment 42

13 years ago
Created attachment 162252 [details] [diff] [review]
updated patch
Attachment #157758 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #162252 - Flags: superreview?(jag)
Attachment #162252 - Flags: review?(db48x)

Comment 43

13 years ago
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
Attachment #162252 - Flags: superreview?(jag) → superreview+
Comment on attachment 162252 [details] [diff] [review]
updated patch

r=db48x
Attachment #162252 - Flags: review?(db48x) → review+
(Assignee)

Updated

13 years ago
Attachment #162252 - Flags: approval-aviary?
(Assignee)

Updated

13 years ago
Attachment #157799 - Attachment is obsolete: true

Comment 45

13 years ago
Comment on attachment 162252 [details] [diff] [review]
updated patch

a=asa
Attachment #162252 - Flags: approval-aviary? → approval-aviary+
checked in
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Keywords: fixed-aviary1.0
Resolution: --- → FIXED
interesting... bug 258185's patch is somewhat similar
Blocks: 258185
verified with windows firefox branch build 2004102607
Status: RESOLVED → VERIFIED

Comment 49

13 years ago
And my comment there about aReferrer applies here too :-/
Product: Browser → Seamonkey

Comment 50

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

13 years ago
(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

13 years ago
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 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.
Attachment #162252 - Flags: approval1.7.6?
*** Bug 275487 has been marked as a duplicate of this bug. ***

Updated

13 years ago
Attachment #157758 - Flags: superreview?(jag)

Updated

13 years ago
Attachment #161256 - Flags: review?(db48x)

Comment 55

13 years ago
Comment on attachment 162252 [details] [diff] [review]
updated patch

a=mkaply for 1.7.6 as this is a firefox/mozilla compat issue.
Attachment #162252 - Flags: approval1.7.6? → approval1.7.6+
checked attachment 161256 [details] [diff] [review] into the MOZILLA_1_7_BRANCH
Keywords: fixed1.7.6
(Assignee)

Comment 57

13 years ago
*** Bug 285609 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

3 years ago
Assignee: db48x → florian
You need to log in before you can comment on or make changes to this bug.