152156 - filepicker dialog freezes mozilla redraw

Status: RESOLVED WONTFIX

(Core :: XUL, defect)

Description

[Sergei Dolgov]

When BeOS native BFilePanel is called from Menu->Open File, it blocks both
access to browser and its repainting. Until BFilePanel is closed.
dunno yet, what to revise - nsFilePickerBeOS or "general" XPFE menu code

Comment 1

[Daniel Switkin]

This is true of the Save As panel as well, as you might expect. I agree it's
pretty lame and annoying.

Comment 2

[Paul]

one way to fix this would be to use the xp panel, instead of the BFilePanel. 
then mozilla would control everything, as we all know it likes to do :)

Comment 3

[Paul]

I have seen this under Linux as well.  Try changing the print command from lpr
to kprinter (if running kde) to get the KDE print dialog.  When it is run, all
drawing within mozilla is stopped, until the dialog is closed.

btw, I don't think we should go to the XPFileDialog

Comment 4

[Sergei Dolgov]

This problem is defferent form linux one, and "shouldn'be" - look at other BeOS
apps using FilePanel. Problem is that RefsReceived message processing is done
inside BFilePanel derived class itself, insted processing it somewhere outside,
e.g. nsWindowBeOS.cpp code. And it needs usage of semaphores to block all other
events until FilePanel gets this message (or cancel). Anyway, this problem isn't
so important, but still may leave in TODO_IN_FUTURE list

Comment 5

[Niels Reedijk]

Moving todo for Firefox 3.0 (unfortunately).

Comment 6

[Niels Reedijk]

I've got an idea on how to solve this. Assigning it to me as a reminder.

Comment 7

[Niels Reedijk]

Attachment: Concept Patch

Added a concept patch.

This patch would in theory solve the problem, it's a solution the BeBook also mentions, however, it doesn't work. I think that's due to the fact that the parent offered in InitNative() isn't really the parent, but rather one of the ghost windows.

Anyway, I need some help by the experts to continue this search.

Comment 8

[Niels Reedijk]

Attachment: Fix, still needs work

This patch _should_ work, but it doesn't entirely. The problem shows up when trying to pick a file in the browser window itself. Picking files from the File menu and then Open File, and picking files from Browse windows in the preferences work without any issues, but from pages (such as bugzilla), the nsIAppshell associated with the nsIWidget is null.

So basically, the issue revolves on whether it's predictable when the appshell is null, and where to get it if that's the case.

Comment 9

[Niels Reedijk]

Attachment: This one works, but is ugly

Here's another iteration. This one works! I thought I'd share it with the world. I'm going to do another one. Sergei pointed out that there is a better fix, which I'll try out in a moment.

Comment 10

[Niels Reedijk]

Attachment: Iteration #4

This is another iteration, which works, however, after opening a file picker it becomes very slow for some reason. I really don't know what the reason is. I do know that this version is stylistically (when I tailor it a bit more) better than the third iteration.

Comment 11

[Niels Reedijk]

Just a thought on why #4 might not work as espected. Perhaps the do_GetService creates a new appshell. In this process, the old port is deleted, thus discarding all the messages waiting there. A new one is created, but this one misses some important messages that slows down the rest of the app. 

Is this plausible?

Comment 12

[tqh]

It shouldn't create another I think. Will read code tomorrow.

Comment 13

[Niels Reedijk]

Hi there, it's been a while.

This weekend I dug up my old computer which contained the firefox source. Just to let you know how things have been going:

I'm pretty sure that attachment 244713 [details] [diff] [review] is _the_ way to fix this issue in Firefox 2.

There are two issues:
1. If the file dialog is being dragged, it still leaves drag traces in the firefox window, though as soon as you release the mousebutton it will redraw.
2. If the file picker is initiated from a web page, Firefox crashes. I came to the conclusion that this is because we request the AppShell, which is a native service in the browser itself, but it's probably 'locked off' when it is called from a web page (which seems wise).

Solutions to 2:
a) Don't redraw the window when there is no appshell.
b) Dig deeper and find a way to get the pointer anyway. Perhaps there's an option to force retrieving the appshell. I don't know about this though.

Niels

Comment 14

[Sergei Dolgov]

(In reply to comment #13)
> Hi there, it's been a while.

I think problem lies somewhere else, though.
When you try to open file dialog window, there is loop of calls for nsAppShell::GetNativeEvent/DispatchNativeEvent starting to spin. While for "regular" moz-windows/widget we don't hook there (AppShell "sits" in ::Run() method loop in this case).

But problem is that our GetNativeEvent is stub atm - no real code there. And code in DispatchNativeEvent is somewhat copycat of code in Run() - which don't look best solution for this case.

Comment 15

[Sergei Dolgov]

I solved here this problem, mathing way used by XUL modal windows.
It probably needs bit updated nsAppShell.cpp for BeOS, but may work with existing one too.

Semaphores in FilePicker WaitForSelection are replaced by while() loop in ::Show() method, at place where we call WaitForSelection.
Code in Show() before and Loop looks like that:
   nsCOMPtr<nsIAppShell> mAppShell(do_CreateInstance(kAppShellCID));
   NS_ENSURE_TRUE(mAppShell, NS_ERROR_FAILURE);
   mAppShell->Create(0, nsnull);
   mAppShell->Spinup();
----------------------
	ppanel->Show();
	while(!ppanel->WaitForSelection())
	{
	       void* data;
       PRBool isRealEvent;
       PRBool processEvent;
       nsresult rv = NS_OK;
       rv = mAppShell->GetNativeEvent(isRealEvent, data);
       if(NS_SUCCEEDED(rv))
       {
//         window->ModalEventFilter(isRealEvent, data, &processEvent);
//         if(processEvent)
           mAppShell->DispatchNativeEvent(isRealEvent, data);
       }
	};

and BFilePanel::WaitForSelection is simple bool now:
bool nsFilePanelBeOS::WaitForSelection()
{
	return mIsSelected;
}

Comment 16

[Doug Shelton]

I tested this code in 1.8 branch and it seems to work.  Opening the native file picker panel no longer freezes the main window behind it.  I made no changes to nsAppShell.

Comment 17

[Sergei Dolgov]

(In reply to comment #16)
> I tested this code in 1.8 branch and it seems to work.  Opening the native file
> picker panel no longer freezes the main window behind it.  I made no changes to
> nsAppShell.
> 
That's nice. So we can work on this bug and appshell perfection separately

Comment 18

[Niels Reedijk]

Do you want me to continue working on this? I might create some time for it, if you want to. (Though I won't mind you taking it up either).

Comment 19

[Sergei Dolgov]

(In reply to comment #18)
> Do you want me to continue working on this? I might create some time for it, if
> you want to. (Though I won't mind you taking it up either).
> 

As this exact issue is heavily related to problems I'm investigating for moment, I prefer to take this one.

But I will be grateful, if you, after we fix this bug, will look onto other filepicker problems. E.g. we lack additional but essential elements in our filepicker - like drop-down list with 3 versions how to save web-page - it needs additions to standard BFilePanel, and also filters - by extensions and by mime. 

Comment 20

[Niels Reedijk]

Okay, I'm sure you'll be able to perfect this patch.

I'm glad the work I've put in this will get a good use.

And if I've got some more time and I feel like working on Firefox, I'll have a look at the other issues regarding the filepicker. 

Comment 21

[Sergei Dolgov]

(In reply to comment #20)
> Okay, I'm sure you'll be able to perfect this patch.
> 
> I'm glad the work I've put in this will get a good use.
Nielx, btw, do you have idea, what for our file picker acquires semaphore here:
http://lxr.mozilla.org/mozilla1.8/source/widget/src/beos/nsFilePicker.cpp#403
?
I removed all semaphore handling from code and seems working OK, but wondering if it has some deeper idea behind?:)

Comment 22

[Niels Reedijk]

No, there's no deeper meaning. I have looked at some other code before devising this patch, and I'm pretty sure it's based on something someone else has done. I think that person chose to use semaphores because with acquire_sem_etc() you can set a timeout (so you don't have to snooze(...)), and it would also return immediately if the user selected an item.

But yes, this can be easily implemented not using a semaphore.

Comment 23

[Sergei Dolgov]

Attachment: patch

allows proper update of parent window.

Doug, if it is ok for you, set approval request after reviewing

Comment 24

[Doug Shelton]

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

Code works as expected - resolves bug.  Patch applies cleanly but needs additional change to nsFilePicker.h

Comment 25

[Doug Shelton]

Attachment: additional changes needed for Sergei's patch to work.

Comment 26

[Sergei Dolgov]

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

no r for incomplete patches

Comment 27

[Sergei Dolgov]

Comment on attachment 296417 [details] [diff] [review]
additional changes needed for Sergei's patch to work.

no need for subclassing picker as Blooper, will prepare proper patch this weekend, thanks for your efforts

Comment 28

[Doug Shelton]

(In reply to comment #26)
> (From update of attachment 294856 [details] [diff] [review])
> no r for incomplete patches
> 
thank you for clarifying policy.  I was trying to move things along but understand now, the entire patch must be present to receive a "+"

Comment 29

[Sergei Dolgov]

> thank you for clarifying policy.  I was trying to move things along but
> understand now, the entire patch must be present to receive a "+"
actually I'm more worried now about another patch - for native type recognition bug. As second review for that is stalled for some reason. As it touches code outside BeOS-only parts, we need that look from outside there, while bugs in widget/src/beos are totally in our responsibility (except approvals, yeah)

 

Comment 30

[Sergei Dolgov]

Attachment: complete patch

Doug, please test if this one works as expected

Comment 31

[Doug Shelton]

(In reply to comment #30)
> Created an attachment (id=296685) [details]
> complete patch
> 
> Doug, please test if this one works as expected
> 
This patch applies cleanly to 1.8 branch and works as expected.  w00t!  
Patch does not apply cleanly to trunk - the nsFilePicker versions appear to be slightly different; no way to test trunk for now, either because of other build issues.  This would be a good patch to add to 1.8 branch for Firefox 2.0.0.12.

Comment 32

[Sergei Dolgov]

Comment on attachment 296685 [details] [diff] [review]
complete patch

r=?

set also approval request, when reviewed

Comment 33

[Sergei Dolgov]

Comment on attachment 296685 [details] [diff] [review]
complete patch

a=?

Comment 34

[Samuel Sidler (old account; do not CC)]

Comment on attachment 296685 [details] [diff] [review]
complete patch

Approved for the 1.8 branch; a=ss for release drivers. Please land as soon as possible.

Comment 35

[Sergei Dolgov]

Checking in mozilla/widget/src/beos/nsFilePicker.cpp;
/cvsroot/mozilla/widget/src/beos/nsFilePicker.cpp,v  <--  nsFilePicker.cpp
new revision: 1.28.12.2; previous revision: 1.28.12.1
done
Checking in mozilla/widget/src/beos/nsFilePicker.h;
/cvsroot/mozilla/widget/src/beos/nsFilePicker.h,v  <--  nsFilePicker.h
new revision: 1.12.12.1; previous revision: 1.12
done 

Comment 36

[Doug Shelton]

Thank you, Sergei!  

Comment 37

[juan becerra [:juanb]]

Sergei, could you help us verify this bug fix using the release candidate for Fx 2.0.0.12 on BeOS?

Comment 38

[Doug Shelton]

(In reply to comment #37)
> Sergei, could you help us verify this bug fix using the release candidate for
> Fx 2.0.0.12 on BeOS?
> 
Verified fixed in 2.0.0.12pre built on 2007-01-27.

Comment 39

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Not really sure if this was FIXED or not, but BeOS is dead.