28781 - linux, win: Back/Forward from context menu crashes browser

Status: VERIFIED FIXED

(Core :: XUL, defect, P1)

Description

[sairuh (rarely reading bugmail)]

hokay, this might really be an XPApps bug, rather than an XPMenus bug --but i'm
not sure since i haven't been able to reproduce it when using Go > Forward (main
menu) or the Forward button in the toolbar. so far only seen on linux and winNT
(opt comm bits, 2000022108); not a problem on mac.

essentially, i got crashes when using either Forward (on linux) or Back (on
winNT) from the context menu. pls let me know if these should be two separate
bugs, or are caused by an existing bug[s]... going through the urls specified
will repro this bug --trying to do this with any old page doesn't always crash,
though.

let me know if you encounter this in another situation, so we could narrow it
down. :-)

recipe:
1. go to the above url.
2. enter 501 East Middlefield in the street address input field and 94043 in the
city/state/zip field.
3. click the Get Map button.
4. in the resulting page with the mapbring up context menu, select Back
result: winNT crashes at this point (except once, when it did crash at step 5);
sometimes linux crashes here too.
5. if the browser doesn't crash, you'll be back at the above URL (with filled
fields, o'course). now bring up the context menu and select Forward.
result: now the browser crashes (typically linux).

hokay, here are the Talkback reports...

for going Back on winNT:
http://cyclone/reports/incidenttemplate.CFM?reportID=124&style=0&tc=35&cp=1&ck1=SUser+email+address&cd1=%25sairuh%40netscape%2Ecom%25&co1=like&bbid=5718362

for going Back on winNT (similar to above, tho happened only once):
http://cyclone/reports/incidenttemplate.CFM?reportID=124&style=0&tc=36&cp=4&ck1=SUser+email+address&cd1=%25sairuh%40netscape%2Ecom%25&co1=like&bbid=5707906

for going Back on linux:
http://cyclone/reports/incidenttemplate.CFM?reportID=124&style=0&tc=36&cp=1&ck1=SUser+email+address&cd1=%25sairuh%40netscape%2Ecom%25&co1=like&bbid=5718587

for going Forward on linux (very similar to going Back on linux, too):
http://cyclone/reports/incidenttemplate.CFM?reportID=124&style=0&tc=36&cp=3&ck1=SUser+email+address&cd1=%25sairuh%40netscape%2Ecom%25&co1=like&bbid=5708012

Comment 1

[sairuh (rarely reading bugmail)]

beta1...?

Comment 2

[Mike Pinkerton (not reading bugmail)]

talkback stack points to imagelib....

Comment 3

[neeti]

I get the following stack trace, when we crash. This is same stack trace we for 
bugs 28341 and 28602.

nsFrameImageLoader::DamageRepairFrames(const nsRect * 0x0012fb80) line 558 + 33 
bytes
nsFrameImageLoader::Notify(nsIImageRequest * 0x02b7f520, nsIImage * 0x02c4f690, 
nsImageNotification nsImageNotification_kPixmapUpdate, int 0, int 0, void * 
0x0012fbbc) line 420
ns_observer_proc(void * 0x02b7d1f0, long 4, void * 0x0012fc30, void * 
0x02b7f520) line 97
XP_NotifyObservers(OpaqueObserverList * 0x02b7fc40, long 4, void * 0x0012fc30) 
line 259 + 28 bytes
il_pixmap_update_notify(il_container_struct * 0x02bd0090) line 307 + 18 bytes
il_flush_image_data(il_container_struct * 0x02bd0090) line 218 + 9 bytes
ImgDCallbk::ImgDCBFlushImage(ImgDCallbk * const 0x02bd0430) line 162 + 12 bytes
il_gif_write(il_container_struct * 0x02bd0090, const unsigned char * 0x0227a094, 
long 0) line 1502
process_buffered_gif_input_data(gif_struct * 0x02c4f850) line 669 + 16 bytes
gif_delay_time_callback(void * 0x02bd0090) line 726 + 9 bytes
timer_callback(nsITimer * 0x02fc0160, void * 0x02fc0120) line 71 + 12 bytes
nsTimer::Fire() line 194 + 17 bytes
nsTimerManager::FireNextReadyTimer(nsTimerManager * const 0x019ad290, unsigned 
int 0) line 117
nsAppShell::Run(nsAppShell * const 0x0106a510) line 116
nsAppShellService::Run(nsAppShellService * const 0x010382b0) line 400
main1(int 1, char * * 0x00ca48f0, nsISplashScreen * 0x00000000) line 651 + 32 
bytes
main(int 1, char * * 0x00ca48f0) line 770 + 17 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77f1b304()

Comment 4

[pnunn]

Interesting.
This only happens with the contextmenu back.
The back button works fine. I can see this on
NT and linux.
-p

Comment 5

[pnunn]

Note that you don't have to plugin a street
address to get a map. A zip code is adequate to
get a map.

That aside, the map isn't causing the problem.
Its the ad banners...probably the ad banner redirects
is my current guess.

To test this I copied the image cgi string for the map
and put this into a local html page. I can contextmenu back and
forth for ever and not get a crash.

When I put the banner image url in a simple local html page
and use the contextmenu back I can get a crash with the same
call stack as described in the bug report.

The crash is due to nsFrameImageLoader::DamageRepairFrames() not
getting an ok view in line 549 or 551. (I don't know yet which line
is returning a bogus view.) This doesn't seem too image related, but
I'm gonna keep running with this and see how far I can get.
-P

Comment 6

[pnunn]

here's another update:
To reproduce the problem you only need to view one
of the banners that is animated and try to go back and
forth using the context menu. I'm going to update the URL
to one of the animated menus.
For the record, the old url field was 'http://maps.yahoo.com/'
The new one is:
http://a32.g.a.yimg.com/7/32/31/000/us.yimg.com/a/me/mediabay/driving_1L.gif

-P

Comment 7

[Phil Peterson]

PDT would like to have an ETA date. We'd consider removing the Back button from
the context menu in order to prevent the crash.

Comment 8

[sairuh (rarely reading bugmail)]

i cannot help asking: wouldn't removing Back (and/or Forward) from the context
menu be a rather harsh (albeit temporary) workaround for this problem?

Comment 9

[pnunn]

I don't think we should change the context menu.
I think this bug is a symptom of an important problem
we need to address. I think this bug is a dupe &/or related
to bugs 28341, 28781. 

Though I don't have a fix for it, and have yet to confirm
it, I think this bug is due to an animated image callback 
popping up after we move off of a frame. When the animation
comes up we try to access the frame and get a garbaged view.

The crux of the bug is in the 'reconnect hack' if I am correct.
The timer is connected to the image container. The image container
doesn't know we have moved off the frame. The frame doesn't know
diddly about the timer.

I hope I'm wrong because fixing it will be non trivial.
ETA, I dunno until I confirm or disprove my theory.
-P

Comment 10

[pnunn]

More notes:
This seems to happen with _any_ animated gif.
The animated gif does not need to be a redirect url
like many banners are.

So...Why does the context menu back behave differently from
the chrome button back? After talking to Bill, I tried to see
if the godropdownmenu back would crash like the contextmenu back.
The idea was the animation timer callback was kicking off when
the state of the frame was not yet changed, and so you would get
a crash. The contextmenu back requires a repaint, but the chrome back
does not. The godropdownmenu back also requires a repaint. Unfortunately,
it does not cause a crash like the contextmenu back. One more idea
bites the dust.

-P

Comment 11

[pnunn]

Attachment: call stacks of 3 different cases.

Comment 12

[pnunn]

I've changed the test url to a large animated gif. 

I'm attaching call stacks from 3 different methods of
going back a page. I've placed a break point at 
IL_DestroyGroupContext() in mozilla/modules/libimg/if.cpp.
I start up mozilla. I have home set to the above test url.
For the 3 different methods:
1. Chrome button Back works as expected. We hit IL_DestroyGroupContext().
2. Go menuDropDownList Back works as expected. We hit IL_DestroyGroupContext().
3. Contextmenu Back >>crashes<<. We  do not hit IL_DestroyGroupContext().

Comment 13

[pnunn]

After talking to Bill, Simon, Saari & Pink, I'm reassigning
this bug to Pink. I'll still be looking at the timer/reconnect 
hack issues. Let me know if I help on this one.
-P

Comment 14

[Mike Pinkerton (not reading bugmail)]

what appears to be happening is that when using "back" from the context menu, the 
pres context owned bye the docViewerImpl is leaked (someone else is holding onto 
the pres context, it's not the docViewer because it uses a comPtr). Since the 
dtor for the presContext is never being called, the imageGroup never gets 
destroyed and baaad things happen as the image continues to animate.

Pam is going to use the tools tomorrow (2/25) to see if we can isolate who is 
holding onto the presContext.

Comment 15

[Mike Pinkerton (not reading bugmail)]

p1, accepting, critical.

Comment 16

[a-huntwork]

27249 is a dup of this bug.  This bug affects slashdot.org.

Comment 17

[a-huntwork]

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

Comment 18

[Mike Pinkerton (not reading bugmail)]

Pam, I found this one. I was holding onto a pres shell by accident (forgot to use 
dont_AddRef around GetShellAt()) in the context menu creation code.

The other crashes may be similar...look for leaked presShells in addition to 
presContexts.

Comment 19

[Mike Pinkerton (not reading bugmail)]

fixed. *whew*

Comment 20

[Stephen P. Morse]

YES, YES, YES -- it's fixed.  This also clears up bug 29397 (right mouse button 
causing crash) which has been marked as a dup of bug 28341 rather than this 
one.  I've been spending the last two days investigating that bug and getting 
nowhere.  With the fix that Mike checked in last night, I no longer crash after 
pressing right-clicking. 

Comment 21

[Stephen P. Morse]

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

Comment 22

[sairuh (rarely reading bugmail)]

verif w/opt comm bits on linux and winNT [200002290x].

Comment 23

[Stephen P. Morse]

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

Comment 24

[pnunn]

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