264235 - context menus broken

Status: VERIFIED FIXED

(Core :: Web Painting, defect)

Description

[Tracy Walker [:tracy]]

seen on Firefox Mac trunk build 2004-10-13-07-trunk
(not seeing this on Window or Linux trunk builds from today)

-context click (ctrl+click or right click) on any item

expected results:  the associated context menu appears

tested results: no context menu

perhaps this is in some way related to bug 263992 ?

Comment 1

[Scott Field]

Problem is also occuring in Mozilla 1.8a5 build 2004100705 for Mac OS X (Should
I report as seperae bug?)

Comment 2

[Tracy Walker [:tracy]]

I'll change Product to Browser. Thanks Scott.

Comment 3

[Boris Zbarsky [:bzbarsky]]

If this is happening in 2004-10-07, it's not related to bug 263992.

Can someone with a mac pin down the day this regressed?

Comment 4

[Sébastien Desvignes]

For me it is working in the build 2004101105 (which I am using right now), and
it is broken in the later builds.

Comment 5

[Tracy Walker [:tracy]]

Right, working up to the 11th and then broken on the 12th and 13th on both
Firefox and Mozilla

Comment 6

[Boris Zbarsky [:bzbarsky]]

Sounds like a regression from bug 238493, then.

Comment 7

[David Lawhon]

Also broken on 10/14 Mozilla daily build.

Comment 8

[Tracy Walker [:tracy]]

This is also still broken on Firefox Mac trunk build 2004-10-14-07-trunk

Comment 9

[HARUNAGA Hirotoshi]

*** Bug 264554 has been marked as a duplicate of this bug. ***

Comment 10

[Ben Fowler]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a5) Gecko/20041016
Firefox/0.9.1+

This is still in, but backing out patch 161548 from bug 238493 causes the problem
to vanish.

I will see whether I can narrow the regression down.

Comment 11

[Ben Fowler]

O.K. The patch I backed out touched four directories: gfx, view, layout and 
widget.

Re-doing those parts that touched gfx, layout and widget has not caused the bug 
re-appear.

Continuing ...

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We know what the problem is. See bug 264245.

Comment 13

[Ben Fowler]

I may have narrowed the problem down to 
nsViewManager::ProcessWidgetChanges(nsView* aView) 
and I am certainly in a position to make a patch.

However ...

This bug is "context menus broken" it is marked as Mac OS, and in comment 3,
help was requested.

Bug 264245 is "form popup (combobox) appears in wrong place after scroll",
and appears to need far more and deeper analysis. (Bug 264245 is marked
as blocking this bug).

Maybe this bug is now actually resolved in the sense that work/discussion 
is going on elsewhere. In case submitting a patch to hide the problem
(spackle) might be viewed as an unfriendly act, I shall refrain and 
just watch.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ben, thanks for the help but the work to fix this is going on in bug 264245. If
you could apply the patch I submitted there (attachment 162372 [details] [diff] [review]) and report on
whether that fixes the problem on Mac, that would be extremely useful!

Comment 15

[Ben Fowler]

O.K. I have CVS update'd my tree and applied the patch, waiting for compile ...

Comment 16

[Boris Zbarsky [:bzbarsky]]

I don't think anyone would ever view submitting a patch as an unfriendly act! 
(Though it may get denied review for various reasons, of course.)

Comment 17

[Ben Fowler]

Yes. The pop-up menu problem not in after your patch is applied

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Excellent. Thanks!

Comment 19

[Sébastien Desvignes]

Well, I've applied the patch with Mozilla, and it crashes at startup.
Config: MacOSX 10.3.5
I have this warning in the console with a debug build:
WARNING: dependent window created without a parent, file nsWindowCreator.cpp,
line 128
With gdb, I have:
Program received signal EXC_BAD_ACCESS, Could not access memory.
0x01287b60 in nsMacWindow::StandardCreate(nsIWidget*, nsRect const&,
nsEventStatus (*)(nsGUIEvent*), nsIDeviceContext*, nsIAppShell*, nsIToolkit*,
nsWidgetInitData*, void*) ()

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Try the new patch I just posted in the bug.

Comment 21

[Sébastien Desvignes]

No crash this time, but the context menus are still broken, sorry.

Comment 22

[Paul Bergsagel]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a5) Gecko/20041018

Comment 23

[HARUNAGA Hirotoshi]

*** Bug 264950 has been marked as a duplicate of this bug. ***

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: try this

Try this patch instead --- Thanks!

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: try this v2

ugh, that won't work ... try this

Comment 26

[Martijn Wargers (dead)]

*** Bug 265245 has been marked as a duplicate of this bug. ***

Comment 27

[Sébastien Desvignes]

The patch doesn't work, I'm sorry. The context menus are still missing.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Are you able to debug this for me?

The context menu just doesn't show up at all? Or does it show up in the wrong
place (possibly offscreen)? Can you see it if you activate Expose?

Comment 29

[Sébastien Desvignes]

I'll try to do my best.
Expose doesn't show popup windows, so I can't use it to know if the popup is
missing or offscreen.
I see no context menu at all.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Does the Mac have some kind of window system inspection tool that developers can
use to see all the windows that have been created and what their status is?

Comment 31

[Peter Van der Beken [:peterv]]

Not sure if there's an app, there's a DebugPrintWindowList function useable from
gdb. I haven't used it myself though.

Comment 32

[Sébastien Desvignes]

Well, I have found that the Quartz Debug application can list the windows, but
the list is static. To refresh you need to switch to Quartz Debug, and doing
that dismiss popup windows.

Comment 33

[Peter Van der Beken [:peterv]]

I hit the assertion.

###!!! ASSERTION: Popups should not be hooked into nsIWidget hierarchy:
'!aInitData || aInitData->mWindowType != eWindowType_popup || !aParent', file
/Volumes/Source/mozilla/libxml/mozilla/widget/src/mac/nsMacWindow.cpp, line 281

Comment 34

[jhp (no longer active)]

Attachment: printf log

Put some printfs in nsMacWindow.cpp for the trunk with this patch, and compared
against Moz 1.7.  As you can see, they are very different.  I don't know why we
get all those Move calls; I even get them when moving my mouse across the page.
 roc, any idea why those are there?

As for the context menu, it's created in the wrong location and sized to 0,0. 
Then later on it is moved to the correct location (I think), and sized to the
correct size.  Maybe this is causing it to not appear?

Comment 35

[Sébastien Desvignes]

Strange, I don't assert. (I have checked that the patch is really applied)

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Javier, it looks like it is being positioned and sized correctly. Can you dig
deeper to see where it actually is placed according to Quartz, and why Quartz
isn't displaying it?

Comment 37

[jhp (no longer active)]

Hmm, seems nsMacWindow::Show() isn't being called with the new patch (was called
just fine in 1.7).  Still investigating why.

Comment 38

[jhp (no longer active)]

Sorry, nsMacWindow::Show() is getting called, but none of the Mac funtions to
actually show the window are getting called.  This patch fixed it:

   // we need to make sure we call ::Show/HideWindow() to generate the 
   // necessary activate/deactivate events. Calling ::ShowHide() is
   // not adequate, unless we don't want activation (popups). (pinkerton).
-  if ( bState && !mBounds.IsEmpty() ) {
+  if ( bState ) {
 #if TARGET_CARBON
     if ( mIsSheet && parentWindowRef ) {
         WindowPtr top = GetWindowTop(parentWindowRef);


This line was checked in by pinkerton back in the day, but I saw no reference as
to why the mBounds check was added in the bugs.  Pinkerton, do you remember any
of this?

Comment 39

[Mike Pinkerton (not reading bugmail)]

one of two reasons:
- gecko loves to create and show windows of no size, which on mac show up on the
screen. 
- a bug on early versions of OS X that crashed when you showed a window with no
size.

either are just as likely candidates. what was my checkin comment?

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Javier, is it possible to do the WindowShow call when we Resize() to a non-empty
rect?

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I will look into why these extra Move and Resizes are happening, but they
*should* work, and if they only fail to work on Mac, then Mac's bound to get
broken again later.

Comment 42

[jhp (no longer active)]

Pinkerton, your comment was "remove special-case activation code for hidden
window, let the OS do it. punts hidden window offscreen on mac so it can be
visible yet of non-zero width/height for osx. bugs 70355/70388."

roc, I don't think we want to show the window during Resize(), since Mozilla
sometimes resizes the window before wanting to display it, and then somewhere
down the line calls Show().  At least, that's what it looks like to me.

Comment 43

[jhp (no longer active)]

Some more printf output:

MOZILLA 1.7:
::StandardCreate() mWindowPtr=0x129a3650
::Show(bState=FALSE) mWindowPtr=0x129a3650 mBounds=0 0 0 0
Set mBounds to 2 2 0 0
Set mBounds to 2 2 187 210
::Resize(aW=192, aH=215) mWindowPtr=0x129a3650
Set mBounds to 0 0 192 215
::Show(bState=TRUE) mWindowPtr=0x129a3650 mBounds=0 0 192 215

TRUNK W/ PATCH:
::StandardCreate() mWindowPtr=0x122a93f0
::Show(bState=FALSE) mWindowPtr=0x122a93f0 mBounds=0 0 0 0
::Resize(aW=0, aH=0) mWindowPtr=0x122a93f0
::Show(bState=TRUE) mWindowPtr=0x122a93f0 mBounds=0 0 0 0
::Resize(aW=192, aH=233) mWindowPtr=0x122a93f0
Set mBounds to 0 0 192 233

So with the new patch, we are getting a Resize to 0,0 before the call to
Show(TRUE), with the actual Resize coming afterwards.

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So can you record what the show state is according to Mozilla (whether the last
Show call was PR_TRUE or PR_FALSE), and then in Resize if the size is non-zero
and the show state is PR_TRUE, do WindowShow?

Comment 45

[jhp (no longer active)]

roc, I have no doubt that the Mac code can be changed, I'm just wondering why we
would need to.  Why does Resize get called twice now (and out of order), where
previously it was called once with the correct width and height, right before
Show()?

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know yet. But the sequence of calls, while suboptimal, is not invalid
... and I just want to get the tree reopened before I start poking around and
changing things again.

Comment 47

[jhp (no longer active)]

Attachment: show/resize patch

This is a patch against the previous patch.  Basically, I created a new member
variable (mShown) that is only set when the window is actually made visible. 
Then in Resize(), I check if the window is supposed to be visible (mVisible ==
TRUE) but hasn't been shown yet (mShown == FALSE).  If that's the case, then I
call Show(TRUE).

Comment 48

[jhp (no longer active)]

Comment on attachment 162930 [details] [diff] [review]
show/resize patch

peterv, what do you think of this?

Comment 49

[Martijn Wargers (dead)]

*** Bug 265653 has been marked as a duplicate of this bug. ***

Comment 50

[Ben Fowler]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a5) Gecko/20041023
Firefox/0.9.1+

After I have done a CVS update, the buggy behaviour is back in. Should it be?

It is very awkward trying to test a browser without toolbar menus, URL
completion et cetera!

Incidentally, I am seeing these messages:
###!!! ASSERTION: couldn't set a property on the window, event handling will
fail: 'err == noErr', file ../../../../src/widget/src/mac/nsMacWindow.cpp, line 503

Is this likely to be significant of anything?

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ben, yes, we're currently waiting on peterv to review an additional fix to get
Mac working properly.

Comment 52

[Bruce Davidson]

*** Bug 265778 has been marked as a duplicate of this bug. ***

Comment 53

[David Lawhon]

Could someone please review the patch for Bug #264235? It's been waiting a week to be reviewed, and 
that's left Mac users of Mozilla without a new daily build that supports contextual menus since Oct. 12.

Comment 54

[Mike Pinkerton (not reading bugmail)]

i'd like an answer to the question raised in javier's comment #45. why is resize
now called twice? 

Comment 55

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know, but there's nothing wrong with Resizing it twice.

Comment 56

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 162930 [details] [diff] [review]
show/resize patch

r=pink

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 162930 [details] [diff] [review]
show/resize patch

I think there may be a problem if we Resize to (0,0) to hide the window, but I
won't hold this up for that.

Comment 58

[jhp (no longer active)]

Checked in to trunk.  roc, what problem with resizing to (0,0)?  I put an
assertion right before the Show() call in Resize(), and it got hit quite a bit,
but I didn't notice any adverse effects.  I did not check in the assertion.

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I thought that the Mac would barf on a (0,0) sized "visible" popup, so we'd have
to hide it, but if not, great. Thanks!

Comment 60

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Well, you didn't really test it because we currently don't resize anything to
(0,0) while it's showing. If something changes to make that happen, this might
break.

Comment 61

[jhp (no longer active)]

Attachment: resize check for (0,0)

Ah, I see what you're saying.  How about this patch then?

Comment 62

[jhp (no longer active)]

Reopening in order to get resize to (0,0) check reviewed.

Comment 63

[jhp (no longer active)]

Attachment: resize check for (0,0) (diff -w)

It turns out the system call ::SizeWindow will not do anything if width and
height are 0.  So if that's the case, we just hide the window.

Comment 64

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 163853 [details] [diff] [review]
resize check for (0,0) (diff -w)

Thanks Javier. This is great.

Comment 65

[jhp (no longer active)]

Comment on attachment 163853 [details] [diff] [review]
resize check for (0,0) (diff -w)

Pinkerton pointed out a problem with this patch: if we resize a visible window
to (0,0), the call to Show(false) will change the window to not visible
(mVisible == false).  So if we later resize to non-zero, it will not display.

Comment 66

[jhp (no longer active)]

Attachment: resize check for (0,0) v1.1 (diff -w)

Ok, I think this should do it.	This is the short, hacky patch.  A cleaner
version would require me to do something like in gtk2/nsCommonWidget.cpp; but
that's somewhat involved.

Comment 67

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 163862 [details] [diff] [review]
resize check for (0,0) v1.1 (diff -w)

r=pink

Comment 68

[jhp (no longer active)]

Comment on attachment 163862 [details] [diff] [review]
resize check for (0,0) v1.1 (diff -w)

One last time, roc!

Comment 69

[Boris Zbarsky [:bzbarsky]]

Javier, does that patch fix the various dependencies of this bug?  That is, is
it safe to start making other viewmanager changes now?  ;)

Comment 70

[jhp (no longer active)]

*** Bug 264229 has been marked as a duplicate of this bug. ***

Comment 71

[jhp (no longer active)]

*** Bug 264612 has been marked as a duplicate of this bug. ***

Comment 72

[jhp (no longer active)]

*** Bug 265811 has been marked as a duplicate of this bug. ***

Comment 73

[jhp (no longer active)]

*** Bug 266293 has been marked as a duplicate of this bug. ***

Comment 74

[jhp (no longer active)]

bz: change viewmanager at will!

Comment 75

[Paul Bergsagel]

Did the fix for this bug cause another bug. I downloaded the Suite nightly for
the Mac (10/29/04) and it was too buggy to use. I experienced numerous quits or
hangs which caused corruption in my profile (fortunately I back up regularly)
and caused the OS to become unstable and once the OS itself froze (usually never
does this). Upon reverting to an earlier stable nightly I had to replace my
profile with my backup before Mozilla would launch. I filed a bug for this issue
in case the fix for this bug is not the cause: bug 266806 "The Mozilla
Application Unexpectedly Quits or Hangs".

Comment 76

[Janaki Kuruppu]

(In reply to comment #75)
> Did the fix for this bug cause another bug. I downloaded the Suite nightly for
> the Mac (10/29/04) and it was too buggy to use. I experienced numerous quits or
> hangs which caused corruption in my profile (fortunately I back up regularly)
> and caused the OS to become unstable and once the OS itself froze (usually never
> does this). Upon reverting to an earlier stable nightly I had to replace my
> profile with my backup before Mozilla would launch. I filed a bug for this issue
> in case the fix for this bug is not the cause: bug 266806 "The Mozilla
> Application Unexpectedly Quits or Hangs".

Well, on Mac running OS X 10.3.5 with build (Mozilla/5.0 (Macintosh; U; PPC Mac
OS X Mach-O; en-US; rv:1.8a5) Gecko/20041030) all the bugs seem to be
resolved....i can see the context-sensitive menus, fill in forms with pull-down
menus, and although i've only been using it for a couple of minutes, no major
crash so far........

Comment 77

[Boris Zbarsky [:bzbarsky]]

*** Bug 266131 has been marked as a duplicate of this bug. ***

Comment 78

[Paul Bergsagel]

I also thought all the bugs were fixed. Then I used the app for more than 20 -
30 minutes and the longer I used the application the more unstable it became
until it came to the point that I felt it was safer to revert to a known stable
nightly of 1.8a4 than risk regular crashes and corruption to my profile.

The instability creeps up gradually. At first the application appears stable and
all bugs appear fixed. The longer the application is used the more unstable the
application becomes and the more frequent the crashes appear.

BTW I am using a B/W G3 with MacOS X 10.3.5 if this makes any difference.

I still suspect that the fix for this bug may have created new bugs causing the
application to develope instability over time.

Comment 79

[Sébastien Desvignes]

There is no proof that the instability you experienced is caused by this patch.
If you think there is a problem of stability in the recent nightlies, create a
new bug, and use talkback to collect information about the crashes.

Comment 80

[Tracy Walker [:tracy]]

verified on Mac trunk build 2004-11-10-06-trunk