Saving image does not open the save location window sometimes

RESOLVED FIXED in mozilla1.8rc1

Status

()

Core
DOM
P2
minor
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Pascal Lee, Assigned: mrbkap)

Tracking

({fixed1.8, regression})

Trunk
mozilla1.8rc1
fixed1.8, regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 -
blocking1.8rc1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050907 Firefox/1.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050907 Firefox/1.4

when right clicking on an image in a tab with only the image(like
"http://www.mywebsite.com/image.jpg")to use the "save image as" function,
sometimes the save location window does not open especially whan a lot of tabs
are opened with different images.

Reproducible: Sometimes

Steps to Reproduce:
1.Go to an website with an image gallery like for example
"http://www.freeimages.co.uk/galleries/space/planets/index.htm".
2.right click on each of the image to "open link in a new tab".
3.now select each tab and right click to use the "save image as" funtion and
after choosing the location and saving the image click on the red "x" to close
the tab.
4. repeat this with all the other opened tab.
Actual Results:  
Sometimes the "save image as" function does not brings the "save location
window" but this rarely happens.

Expected Results:  
using the "save image as" function should have always open the save location window.

Doing a refresh by using the "reload current page" button will afterward allow
the "save location window" to open and thus permit the image to be saved. This
does not happen on specific websites but on all types of websites.

Comment 1

12 years ago
I get the prolem on the third image I try to save. 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050916 Firefox/1.4
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050917
Firefox/1.6a1 ID:2005091723

When this happens I see two errors appear in the JS Console:

Error: searchStr has no properties
Source File: chrome://browser/content/browser.js
Line: 4478

Error: gContextMenu has no properties
Source File: chrome://browser/content/browser.xul
Line: 1
Regression between 1.9a1_2005090612 and 1.9a1_2005090620.

Comment 4

12 years ago
I see this in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050918
Firefox/1.4 too.
Keywords: regression
OS: Windows XP → All
regressionwindow for Trunk :
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&filetype=match&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-09-06+11%3A30%3A00&maxdate=2005-09-06+20%3A30%3A00&cvsroot=%2Fcvsroot

Ria, do you happen to have the regressionwindow for branch aswell (roughly) ?
Just days is fine, it would save me a lot of time not having to try 100's of
builds to find it.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 6

12 years ago
WFM - Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20050914
SeaMonkey/1.1a 
Downloaded over 30 images could not reproduce.
(In reply to comment #5)
Sure, branch: 1.8b4_2005090605 - 1.8b4_2005090613.
thanks
for branch:
http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?treeid=default&module=AviarySuiteBranchTinderbox&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-09-06+05%3A00%3A00&maxdate=2005-09-06+13%3A00%3A00&cvsroot=%2Fcvsroot

thinking of Bug 307153
-> cc Aaron Leventhal

Comment 9

12 years ago
reloading the image is the only workaround I found for this bug.

Comment 10

12 years ago
Couldn't ship with it, thus nominating for 1.8b5.
Flags: blocking1.8b5?

Comment 11

12 years ago
->aaron
Assignee: nobody → aaronleventhal

Updated

12 years ago
Version: unspecified → Trunk

Comment 12

12 years ago
I don't think this is bug 307153. That bug just restored code we orginally had
before the checkin for bug 258285, but added some null checks.

I can't really duplicate the results in this bug. Can someone who can duplicate
the testcase in this bug try backing out various patches to see which one it
really is?
Assignee: aaronleventhal → nobody
Whiteboard: [ETA: need to find out exactly what regressed it ]

Comment 13

12 years ago
Hey guys found another way to workaround this issue.
When the save dialog fails to open, select the download manager window then
reselect the browser window and try again. Its worked everytime for me so far.

Comment 14

12 years ago
I think we need to better understand what's going on here. MozQA folks, can you
help us get a clean testcase for this?
Keywords: qawanted

Comment 15

12 years ago
1. load some images in consecutive tabs
2. select the left most tab with an image
3. right click, save image
4. close that tab (I used middle click)
5. right click, save image - no file picker
so far this is 100% reproducable for me

JavaScript error: chrome://browser/content/browser.xul, line 1: gContextMenu has
no properties
###!!! ASSERTION: cannot get parentTop: 'parentTop', file
d:/mozilla/dom/src/base/nsGlobalWindow.cpp, line 5184

1. load some images in consecutive tabs
2. select the right most tab with an image
3. right click choose save image
4. close that tab (I used middle click)
5. right click choose save image
6. if you get a file picker close the tab and repeat step 5
until no file picker (often the second time)

JavaScript error: chrome://browser/content/browser.xul, line 1: gContextMenu has
no properties
WARNING: Weird, we're finalized with a null mJSObject?, file
d:/mozilla/dom/src/base/nsGlobalWindow.cpp, line 1624
###!!! ASSERTION: cannot get parentTop: 'parentTop', file
d:/mozilla/dom/src/base/nsGlobalWindow.cpp, line 5184

Note that the assertion does not happen until you click back into the window.
This problem exists if you use local images too. This is from a Firefox Trunk
Debug build, http://anchorweb.org/Fx/.mozconfig Mozilla/5.0 (Windows; U; Windows
NT 5.1; en-US; rv:1.9a1) Gecko/20050923 Firefox/1.6a1.
(In reply to comment #12)

> I can't really duplicate the results in this bug. 

The bug is very obvious. This site with larger images for instance: 
http://www.nasa.gov/multimedia/imagegallery/
New profile, no theme installed, saving the second, third or fourth image will
always fail. Smaller images can give a less obvious result.

Updated

12 years ago
Flags: blocking1.8b5? → blocking1.8b5+

Updated

12 years ago
Assignee: nobody → mconnor
(In reply to comment #16)
> (In reply to comment #12)
> 
> > I can't really duplicate the results in this bug. 
> 
> The bug is very obvious. This site with larger images for instance: 
> http://www.nasa.gov/multimedia/imagegallery/
> New profile, no theme installed, saving the second, third or fourth image will
> always fail. Smaller images can give a less obvious result.

Using the Images at the nasa site I was able to reproduce this bug just once by
openening several of the images in new tabs, then folling the steps in comment 15.
Keywords: qawanted
Hardware: PC → All

Comment 18

12 years ago
I still can't duplicate those results. And I don't believe the patch I checked
in would have caused this.

Anyone here know how to back individual patches out and rebuild to see what
broke it?

Comment 19

12 years ago
Major discovery. This only looks like a regression from me because I fixed it
and then broke it again when I reversed part of the change from bug 258285.

It has been broken for a while at least. It is broken in the 7/30 build which is
way before I checked in bug 307153.

Here's the picture I think we probably have:

Prior to checkin of bug 258285 (8/16 and earlier): broken
After checkin of bug 258285, but before reversal of SendFocusBlur focus
suppression (8/17 - 9/6): works
After reversal of SendFocusBlur focus suppression (9/7 - now): broken

So in otherwords when I removed SendFocusBlur window suppression temporarily it
fixed the problem. We had to put that back in because of bug 307153.

We need to find the regression window from before 7/30 !!!

Comment 20

12 years ago
Works in 2/23
Broken in 7/29 (so splitwindow is not the culprit)

Anyone have cycles to find the regression window between those dates?

Comment 21

12 years ago
It still seems to be working in a Mozilla/5.0 (Windows; U; Windows NT 5.1;
en-US; rv:1.8b4) Gecko/20050720 Firefox/1.0+, no bug present.

Comment 22

12 years ago
Works in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4)
Gecko/20050725 Firefox/1.0+

Broken in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4)
Gecko/20050726 Firefox/1.0+
(In reply to comment #22)
> Works in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4)
> Gecko/20050725 Firefox/1.0+
> 
> Broken in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4)
> Gecko/20050726 Firefox/1.0+

http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&filetype=match&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-07-25+05%3A00%3A00&maxdate=2005-07-26+07%3A00%3A00&cvsroot=%2Fcvsroot

Comment 24

12 years ago
The checkin that broke it was for bug 301435, which focuses the selected item in
the download manager. I checked it in for Doron Rosenberg who wrote the patch.

2005-07-25 15:21	aaronleventhal%moonset.net 	mozilla/ toolkit/ mozapps/
downloads/ content/ downloads.js 	1.47 	15/0  	Bug 301435. Focus breaks when
item removed from download manager. Patch by Doron Rosenberg. r+a=mconnor

That code was later moved out of downloads.js to richlistbox.xml in bug 306522:
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/richlistbox.xml#248

This bug can be fixed by removing that line or turning off "Show Download
Manager when a download begins"

Comment 25

12 years ago
(In reply to comment #24)
> This bug can be fixed by removing that line or turning off "Show Download
> Manager when a download begins"

To clarify, removing that line causes problems with keyboard navigation in the
download manager. Specifically, when you alt+click on a link and the download
manager opens with the new item selected, you can't down arrow anymore.

So, still looking for the right fix.

Updated

12 years ago
Whiteboard: [ETA: need to find out exactly what regressed it ] → [ETA: have fix ready to review for trunk but should consider mconnor's low risk patch for branch ]

Comment 26

12 years ago
Created attachment 198189 [details] [diff] [review]
ESM change, not low risk. Put in symmetrical focus unsuppressions for "SendFocusBlur  window switch" & "SendFocusBlur  window switch #2" 

I think Mats may have suggested we do something like this once before. I can't
say it's low risk for branch, but it doesn't regress bug 307153 which is a good
sign. That's the bug which regressed when we tried to remove those
SetSuppressFocus calls.

We may want to take my proposed fix for trunk and consider just using the
wallpaper workaround mentioned by Mconnor for branch. Mconnor, care to post
that?
Assignee: mconnor → aaronleventhal
Status: NEW → ASSIGNED
Attachment #198189 - Flags: superreview?(bryner)
Attachment #198189 - Flags: review?(mats.palmgren)

Comment 27

12 years ago
I think Mike Connor was working on a band aid fix for this issue. At this late
point in the game, we'd probably be more interested in the band aid for the
branch instead of changing nsEventStateManager which is historically very fragile.

Comment 28

12 years ago
(In reply to comment #27)
> I think Mike Connor was working on a band aid fix for this issue. At this late
> point in the game, we'd probably be more interested in the band aid for the
> branch instead of changing nsEventStateManager which is historically very fragile.

I agree with that, but I wanted to post what I think the correct long term fix
is so we know what the alternative is. We may at least want the ESM fix for trunk.
Comment on attachment 198189 [details] [diff] [review]
ESM change, not low risk. Put in symmetrical focus unsuppressions for "SendFocusBlur  window switch" & "SendFocusBlur  window switch #2" 

I'd feel a lot better about this if I knew where the focus unsuppression
happens in the "normal" case right now.  Not in SendFocusBlur, it seems.

Comment 30

12 years ago
(In reply to comment #29)
> (From update of attachment 198189 [details] [diff] [review] [edit])
> I'd feel a lot better about this if I knew where the focus unsuppression
> happens in the "normal" case right now.  Not in SendFocusBlur, it seems.
> 

It happens when a window gets activated. In ESM's NS_ACTIVATE handling it clears
out any focus supressions just to be safe. That's the only way this gets turned
off. Unfortunately NS_ACTIVATE doesn't happen as much in the world of tabbed
browsing.
Comment on attachment 198189 [details] [diff] [review]
ESM change, not low risk. Put in symmetrical focus unsuppressions for "SendFocusBlur  window switch" & "SendFocusBlur  window switch #2" 

ok.
Attachment #198189 - Flags: superreview?(bryner) → superreview+

Comment 32

12 years ago
too risky for this late in the game. 
Flags: blocking1.8b5+ → blocking1.8b5-
Flags: blocking1.8rc1?

Comment 33

12 years ago
still too risky this late in the game. 
Flags: blocking1.8rc1? → blocking1.8rc1-
(In reply to comment #26)
> I think Mats may have suggested we do something like this once before.

For reference: bug 306076 "Combined Patch rev. 2": attachment 195006 [details] [diff] [review].

The patch looks good but I see a difference to what I suggested.
You unsuppress "switch #2" before the EnsureFocusSynchronization() calls
(which calls fc->SetFocusedElement()) so there could potentially be two calls
to UpdateCommands(). I had the unsuppression after to avoid that.

If it still fixes this bug to have them after I would prefer that, unless you
see a problem with it. (sorry for the late review)

Comment 35

12 years ago
Created attachment 199799 [details] [diff] [review]
Avoid double command updates and potential for disagreeing variables by using only one
Attachment #199799 - Flags: review?(mats.palmgren)

Comment 36

12 years ago
Also fixes bug 312227.
Blocks: 312227

Updated

12 years ago
Attachment #198189 - Attachment is obsolete: true
Attachment #198189 - Flags: review?(mats.palmgren)

Comment 37

12 years ago
Comment on attachment 199799 [details] [diff] [review]
Avoid double command updates and potential for disagreeing variables by using only one

Brian, this is an impovement over what you already sr='d
Attachment #199799 - Flags: superreview?(bryner)
Comment on attachment 199799 [details] [diff] [review]
Avoid double command updates and potential for disagreeing variables by using only one

Looks good. r=mats
Attachment #199799 - Flags: review?(mats.palmgren) → review+
Comment on attachment 199799 [details] [diff] [review]
Avoid double command updates and potential for disagreeing variables by using only one

I think it would be much less error-prone if you wrote and used a small
stack-based object that automatically unsuppresses the focus controller when it
goes away.

Also, I assume the mLastFocusedWith change is not related to this bug.

Comment 40

12 years ago
(In reply to comment #39)
> (From update of attachment 199799 [details] [diff] [review] [edit])
> I think it would be much less error-prone if you wrote and used a small
> stack-based object that automatically unsuppresses the focus controller when it
> goes away.
Okay, that is better.

> Also, I assume the mLastFocusedWith change is not related to this bug.
Right. 

New patch coming?  I know, nag nag nag....

/be

Comment 42

12 years ago
Blake's going to take a look at this bug and Aaron's patch too.
Flags: blocking1.8rc1- → blocking1.8rc1?

Updated

12 years ago
Flags: blocking1.8rc1? → blocking1.8rc1+

Updated

12 years ago
Assignee: aaronleventhal → mrbkap
Status: ASSIGNED → NEW
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → Firefox1.5rc1

Updated

12 years ago
Attachment #199799 - Flags: superreview?(bryner)

Comment 43

12 years ago
(In reply to comment #42)
> Blake's going to take a look at this bug and Aaron's patch too.

Thank you. I'm not around much for the next few days.
(Assignee)

Comment 44

12 years ago
Created attachment 200582 [details] [diff] [review]
potential updated patch

I haven't compiled this yet, so there might be silly typos.
(Assignee)

Comment 45

12 years ago
Created attachment 200583 [details] [diff] [review]
updated updated patch
Attachment #200582 - Attachment is obsolete: true
Comment on attachment 200583 [details] [diff] [review]
updated updated patch

+class nsFocusSuppressor {

+  void Suppress(nsIFocusController *controller, const char *reason)

Might be worth documenting that the reason argument is assumed to outlive the instance of this class.

r+sr=jst
Attachment #200583 - Flags: superreview+
Attachment #200583 - Flags: review+
(Assignee)

Comment 47

12 years ago
Created attachment 200584 [details] [diff] [review]
patch for checkin

Updated

12 years ago
Attachment #200583 - Flags: approval1.8rc1?

Updated

12 years ago
Attachment #200583 - Flags: approval1.8rc1? → approval1.8rc1+
(Assignee)

Comment 48

12 years ago
Comment on attachment 200583 [details] [diff] [review]
updated updated patch

Brian, would you give this an after-the-fact look over?
Attachment #200583 - Flags: superreview+ → superreview?(bryner)
(Assignee)

Comment 49

12 years ago
Fix checked into MOZILLA_1_8_BRANCH and trunk. (along with a bustage fix)
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Component: File Handling → DOM
Flags: review+
Product: Firefox → Core
QA Contact: file.handling → ian
Target Milestone: Firefox1.5rc1 → ---
Target Milestone: --- → mozilla1.8rc1
Whiteboard: [ETA: have fix ready to review for trunk but should consider mconnor's low risk patch for branch ]
(Assignee)

Updated

12 years ago
Depends on: 314893
Comment on attachment 200583 [details] [diff] [review]
updated updated patch

Much better, I've thought we should do something like this for awhile.
Attachment #200583 - Flags: superreview?(bryner) → superreview+
You need to log in before you can comment on or make changes to this bug.