Closed
Bug 239472
Opened 19 years ago
Closed 19 years ago
Page Info -> media -> Save As... doesn't work
Categories
(SeaMonkey :: Page Info, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: masayuki, Assigned: florian)
References
Details
(Keywords: fixed-aviary1.0, fixed1.7.6, regression, Whiteboard: [have patch] ready to land)
Attachments
(1 file, 4 obsolete files)
7.07 KB,
patch
|
db48x
:
review+
jag+mozilla
:
superreview+
asa
:
approval-aviary+
mkaply
:
approval1.7.6+
|
Details | Diff | Splinter Review |
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•19 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•19 years ago
|
||
I filed the issue only on Mac as Bug 239535.
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•19 years ago
|
Keywords: regression
Comment 5•19 years ago
|
||
*** Bug 242032 has been marked as a duplicate of this bug. ***
Comment 6•19 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? ;-)
(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•19 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
*** Bug 244623 has been marked as a duplicate of this bug. ***
Comment 10•19 years ago
|
||
It looks like the bug has been resolved in the latest nightly - works fine for me.
Comment 11•19 years ago
|
||
Bug is still there in Mozilla trunk build 2004052709 on WinNT4.
Comment 12•19 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•19 years ago
|
||
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•19 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•19 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•19 years ago
|
||
*** Bug 254984 has been marked as a duplicate of this bug. ***
Comment 17•19 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•19 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.
Comment 19•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
Comment on attachment 157758 [details] [diff] [review] a better patch requesting review
Attachment #157758 -
Flags: review?(db48x)
Updated•19 years ago
|
Flags: blocking-aviary1.0?
Assignee | ||
Comment 23•19 years ago
|
||
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•19 years ago
|
Attachment #157758 -
Flags: review?(db48x)
Comment 24•19 years ago
|
||
Comment on attachment 157799 [details] [diff] [review] patch for Firefox r=db48x
Attachment #157799 -
Flags: review+
Updated•19 years ago
|
Attachment #154948 -
Flags: review?
Comment 25•19 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•19 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•19 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•19 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 29•19 years ago
|
||
Comment on attachment 157758 [details] [diff] [review] a better patch r=db48x on this part too
Attachment #157758 -
Flags: review?(db48x) → review+
Comment 30•19 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•19 years ago
|
Attachment #157799 -
Flags: superreview?(jag)
Assignee | ||
Updated•19 years ago
|
Attachment #157758 -
Flags: superreview?(jag)
Comment 31•19 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•19 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•19 years ago
|
||
Btw, Daniel, with bug 72522 and bug 72524 fixed, should we kill getAbsoluteURL()?
Comment 34•19 years ago
|
||
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•19 years ago
|
||
(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•19 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•19 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•19 years ago
|
Whiteboard: [have patch] ready to land
Comment 38•19 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•19 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•19 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•19 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•19 years ago
|
||
Attachment #157758 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Attachment #162252 -
Flags: superreview?(jag)
Attachment #162252 -
Flags: review?(db48x)
Comment 43•19 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 44•19 years ago
|
||
Comment on attachment 162252 [details] [diff] [review] updated patch r=db48x
Attachment #162252 -
Flags: review?(db48x) → review+
Assignee | ||
Updated•19 years ago
|
Attachment #162252 -
Flags: approval-aviary?
Assignee | ||
Updated•19 years ago
|
Attachment #157799 -
Attachment is obsolete: true
Comment 45•19 years ago
|
||
Comment on attachment 162252 [details] [diff] [review] updated patch a=asa
Attachment #162252 -
Flags: approval-aviary? → approval-aviary+
Comment 46•19 years ago
|
||
checked in
Comment 48•19 years ago
|
||
verified with windows firefox branch build 2004102607
Status: RESOLVED → VERIFIED
Comment 49•19 years ago
|
||
And my comment there about aReferrer applies here too :-/
Updated•19 years ago
|
Product: Browser → Seamonkey
Comment 50•19 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•19 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•19 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 53•19 years ago
|
||
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?
Comment 54•19 years ago
|
||
*** Bug 275487 has been marked as a duplicate of this bug. ***
Updated•19 years ago
|
Attachment #157758 -
Flags: superreview?(jag)
Updated•19 years ago
|
Attachment #161256 -
Flags: review?(db48x)
Comment 55•19 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+
Comment 56•19 years ago
|
||
checked attachment 161256 [details] [diff] [review] into the MOZILLA_1_7_BRANCH
Updated•19 years ago
|
Keywords: fixed1.7.6
Assignee | ||
Comment 57•19 years ago
|
||
*** Bug 285609 has been marked as a duplicate of this bug. ***
Assignee | ||
Updated•9 years ago
|
Assignee: db48x → florian
You need to log in
before you can comment on or make changes to this bug.
Description
•