309027 - Saving image does not open the save location window sometimes

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Pascal Lee]

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

[u63580]

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

Comment 2

[Ria Klaassen (not reading all bugmail)]

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

Comment 3

[Ria Klaassen (not reading all bugmail)]

Regression between 1.9a1_2005090612 and 1.9a1_2005090620.

Comment 4

[Kevin Brosnan]

I see this in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050918
Firefox/1.4 too.

Comment 5

[Peter van der Woude [:Peter6]]

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.

Comment 6

[Kevin Brosnan]

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.

Comment 7

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #5)
Sure, branch: 1.8b4_2005090605 - 1.8b4_2005090613.

Comment 8

[Peter van der Woude [:Peter6]]

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

[Dimitrios]

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

Comment 10

[Dimitrios]

Couldn't ship with it, thus nominating for 1.8b5.

Comment 11

[Asa Dotzler [:asa]]

->aaron

Comment 12

[Aaron Leventhal]

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?

Comment 13

[grunt0r]

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

[Asa Dotzler [:asa]]

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

Comment 15

[Kevin Brosnan]

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.

Comment 16

[Ria Klaassen (not reading all bugmail)]

(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.

Comment 17

[Tracy Walker [:tracy]]

(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.

Comment 18

[Aaron Leventhal]

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

[Aaron Leventhal]

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

[Aaron Leventhal]

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

[Kevin Brosnan]

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

[Kevin Brosnan]

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+

Comment 23

[Peter van der Woude [:Peter6]]

(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

[Aaron Leventhal]

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

[Aaron Leventhal]

(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.

Comment 26

[Aaron Leventhal]

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

Comment 27

[Scott MacGregor]

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

[Aaron Leventhal]

(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 29

[Brian Ryner (not reading)]

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

[Aaron Leventhal]

(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 31

[Brian Ryner (not reading)]

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.

Comment 32

[Asa Dotzler [:asa]]

too risky for this late in the game. 

Comment 33

[Scott MacGregor]

still too risky this late in the game. 

Comment 34

[Mats Palmgren (inactive)]

(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

[Aaron Leventhal]

Attachment: Avoid double command updates and potential for disagreeing variables by using only one

Comment 36

[Aaron Leventhal]

Also fixes bug 312227.

Comment 37

[Aaron Leventhal]

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

Comment 38

[Mats Palmgren (inactive)]

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

Looks good. r=mats

Comment 39

[Brian Ryner (not reading)]

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

[Aaron Leventhal]

(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. 

Comment 41

[Brendan Eich [:brendan]]

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

/be

Comment 42

[Scott MacGregor]

Blake's going to take a look at this bug and Aaron's patch too.

Comment 43

[Aaron Leventhal]

(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.

Comment 44

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: potential updated patch

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

Comment 45

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: updated updated patch

Comment 46

[Johnny Stenback (:jst)]

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

Comment 47

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch for checkin

Comment 48

[Blake Kaplan (:mrbkap) (inactive)]

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

Brian, would you give this an after-the-fact look over?

Comment 49

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into MOZILLA_1_8_BRANCH and trunk. (along with a bustage fix)

Comment 50

[Brian Ryner (not reading)]

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

Much better, I've thought we should do something like this for awhile.