245024 - crash in print preview with onClick in framesets [@ nsFrame::Destroy ]

Status: VERIFIED FIXED

(Core :: Print Preview, defect)

Description

[Antony Mawer]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040206 Firefox/0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040206 Firefox/0.8

I was browsing a suppliers website earlier and went to print something, and when
accidentally clicking on the print preview display, Firefox crashed. The error
is 100% reproducable. I'll attach a reduced testcase in a moment.

Reproducible: Always
Steps to Reproduce:
1. Save the two attached files to disk somewhere
2. Open the frameset.html in the browser
3. Open File -> Print Preview
4. In the Print Preview display, click on the "Click here to crash!" link
Actual Results:  
Firefox crashed

Expected Results:  
Nothing

A few observations:

  * This seems to only occur when there are frames involved
  * If I put <!-- and // --> around the actual JavaScript body (within the
SCRIPT tag), then clicking no longer has any action

While an obscure bug, crashes are never good - I lost all the pages I had open
and the order I was placing. Unfortunately I can't provide the link to the
supplier as you require a dealer login to access the relevant page exhibiting
the bug.

Comment 1

[Antony Mawer]

Attachment: Testcase part 1 - the HTML frameset

Comment 2

[Antony Mawer]

Attachment: Testcase part 2 - the HTML frame

Comment 3

[Antony Mawer]

I neglected to mention that Firefox appears to navigate to the new page
indicated as instructed by the onclick handler in the page. The title of the of
the window changes to that of the mozilla.org site, before the crash occurs
(possibly when it attempts to render the page and has nothing to render into?).

Comment 4

[Olivier Cahagne]

Attachment: On-line testcase

Comment 5

[Olivier Cahagne]

Attachment: TB65580 stack (Linux, 20040518)

Comment 6

[Olivier Cahagne]

in a Linux debug build, this assertion shows up when clicking on the link in
print preview:
###!!! ASSERTION: Someone did not call nsIPresShell::destroy: 'Not Reached',
file nsPresShell.cpp, line 1691
Break: at file nsPresShell.cpp, line 1691

The stack is empty though.

No dupes found, marking NEW.

Comment 7

[Boris Zbarsky [:bzbarsky]]

The problem is probably that print preview sets a listener to cancel events on
the toplevel window only... and events don't bubble/capture across frames within
the content hierarchy.

Print preview needs to be setting the listener on the <browser> containing the
content or on each frame in the window.

Comment 8

[[e-mail]]

bz, any idea as to which listener? The one I guessed at wasn't it :-/

Comment 9

[Boris Zbarsky [:bzbarsky]]

Not offhand... I just recall print preview using some sort of listener setup to
block events...

Comment 10

[[e-mail]]

Ah, actually print preview does not cancel events, you can verify this by
shift-clicking a link in print preview. What it does try to do is to cancel
scripts, but the method it uses doesn't work with frames.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Er.... That sucks.  It should really be cancelling all events with a capturing
chrome listener, if possible... :(

Comment 12

[[e-mail]]

Yes, but you have to be careful not to cancel scrollbar events...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Right.  That can be done by examining the target, I would think....

Comment 14

[[e-mail]]

What about scrollbars in <iframe>/<textarea>/<select>s?

Comment 15

[Boris Zbarsky [:bzbarsky]]

You check not just the target but also the parent... It really shouldn't be hard
to distinguish them.  iframes/Textareas/selects shouldn't be scrollable in print
preview anyway (are they now?)

Comment 16

[[e-mail]]

Ah, no, they're not. So one problem solved...

Comment 17

[[e-mail]]

Attachment: Lame hack

Hmm... this does stop the crash, and scrollbars still work...

Comment 18

[Boris Zbarsky [:bzbarsky]]

Hmm.. So is nsPrintPreviewListener basically unused?

Comment 19

[[e-mail]]

Attachment: Better patch

Just uncommenting the code to add the listeners wasn't sufficient, presumably
because the listeners were bubbling and not capturing.

Let me know if there's a better way to set all these capturing listeners.

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 149656 [details] [diff] [review]
Better patch

Hmm, this is a bit fragile given that as support for new input events is added
(i.e. DOM Level 3 Events' Text event etc) we'll potentially need to add them to
this list. But I can't really come up with a better solution to this right
now...

r=jst

Comment 21

[[e-mail]]

Comment on attachment 149656 [details] [diff] [review]
Better patch

Can't find a better sr, sorry.

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 149656 [details] [diff] [review]
Better patch

sr=jst

Comment 23

[[e-mail]]

Fix checked in.

Comment 24

[Stephen Donner [:stephend] Not actively reading bugmail]

So now scrollbars don't react to click events - bug 245751.

Comment 25

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using the
http://bugzilla.mozilla.org/attachment.cgi?id=149572&action=view testcase in
Print Preview on Windows XP (build 2004-08-16-13).

Comment 26

[Jim Ginn]

Although I cannot reproduce the crash, this patch prevents users from right
clicking / middle clicking / etc. in the print previewed website, correct?

If so, this is something we might want for the Aviary Branch (along with the fix
for bug 245751). Nominating for Aviary 1.0.