161364 - Print preview crashes when page has transparent content

Status: RESOLVED FIXED

(Core :: Print Preview, defect)

Description

[Roland Mainz]

[Spin-off of bug 156043 ("Moz cores when printing [@
nsRenderingContextGTK::Init]")]:
Print preview crashes in blender code when the page has transparent content.

Comment 1

[Roland Mainz]

Taking...

... I can bite myself that I did not catch this xx@@@!!! in bug 156043 ...
grrrr... xx@@@!!!

Comment 2

[Roland Mainz]

Attachment: Patch for 2002-08-04-08-trunk

Death to the lousy error checking... xx@@@!!!...

Comment 3

[Roland Mainz]

Requesting r=/sr= ...

Comment 4

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

What exactly was causing the crash? I don't see what you've changed that's important

Comment 5

[Roland Mainz]

Robert O'Callahan wrote:
> What exactly was causing the crash? I don't see what you've changed that's 
> important

Sorry, I forgot to post the stack trace.
One of 'aSrc' or 'aDst' was |nsnull| in this case - and using a method of an
object which is |nsnull| isn't a good idea... =:-)

Comment 6

[Roland Mainz]

Stack trace looks like this:
-- snip --
t@1 (l@1) signal SEGV (no mapping at the fault address) in rangeCheck at line
112 in file "nsBlender.cpp"
  112     surface->GetDimensions(&width, &height);
(dbx) where
current thread: t@1
=>[1] rangeCheck(surface = (nil), aX = 0, aY = 0, aWidth = 557, aHeight = 128),
line 112 in "nsBlender.cpp"
  [2] nsBlender::Blend(this = 0x846a88, aSX = 0, aSY = 0, aWidth = 557, aHeight
= 128, aSrc = (nil), aDst = (nil), aDX = 0, aDY = 0, aSrcOpacity = 0.3,
aSecondSrc = (nil), aSrcBackColor = 4278190080U, aSecondSrcBackColor =
4294967295U), line 162 in "nsBlender.cpp"
  [3] nsBlender::Blend(this = 0x846a88, aSX = 0, aSY = 0, aWidth = 557, aHeight
= 128, aSrc = 0x748968, aDest = 0x6f8b60, aDX = 0, aDY = 0, aSrcOpacity = 0.3,
aSecondSrc = 0x7cf868, aSrcBackColor = 4278190080U, aSecondSrcBackColor =
4294967295U), line 225 in "nsBlender.cpp"
  [4] nsViewManager::RenderDisplayListElement(this = ???, element = ???, aRC =
CLASS) (optimized), at 0xfba48e34 (line ~1253) in "nsViewManager.cpp"
  [5] nsViewManager::RenderViews(this = ???, aRootView = ???, aRC = CLASS, aRect
= STRUCT, aResult = ???) (optimized), at 0xfba48ae4 (line ~1152) in
"nsViewManager.cpp"
  [6] nsViewManager::Refresh(this = ???, aView = ???, aContext = ???, aRegion =
???, aUpdateFlags = ???) (optimized), at 0xfba47a98 (line ~748) in
"nsViewManager.cpp"
  [7] nsViewManager::DispatchEvent(this = ???, aEvent = ???, aStatus = ???)
(optimized), at 0xfba4a0f8 (line ~1743) in "nsViewManager.cpp"
  [8] HandleEvent(aEvent = ???) (optimized), at 0xfba3b83c (line ~80) in
"nsView.cpp"
  [9] nsWidget::DispatchEvent(this = ???, aEvent = ???, aStatus = ???)
(optimized), at 0xfd3c0690 (line ~1424) in "nsWidget.cpp"
  [10] nsWidget::DispatchWindowEvent(this = ???, event = ???) (optimized), at
0xfd3c030c (line ~1312) in "nsWidget.cpp"
  [11] nsWindow::DoPaint(this = ???, aX = ???, aY = ???, aWidth = ???, aHeight =
???, aClipRegion = ???) (optimized), at 0xfd3c62a4 (line ~4129550) in
"nsWindow.cpp"
  [12] nsWindow::Update(this = ???) (optimized), at 0xfd3c63e8 (line ~4129586)
in "nsWindow.cpp"
  [13] nsWindow::Update(this = ???) (optimized), at 0xfd3c65d0 (line ~4129620)
in "nsWindow.cpp"
  [14] nsWindow::UpdateIdle(data = ???) (optimized), at 0xfd3c5ed8 (line
~4129459) in "nsWindow.cpp"
dbx: warning: can't find file
"/home/gisburn/package-builds/glib/glib-1.2.8/objdir/gmain.lo"
dbx: warning: see `help finding-files'
  [15] g_idle_dispatch(0xfd3c5e58, 0xffbeeff8, 0x0, 0x0, 0x0, 0xffbeef60), at
0xfe7b94b4
  [16] g_main_dispatch(0xffbeeff8, 0xb9e70, 0x1, 0x0, 0x0, 0x0), at 0xfe7b6dc8
  [17] g_main_iterate(0x1, 0x1, 0x5, 0xff3e4270, 0xfd3990cb, 0x18), at
0xfe7b7bcc
  [18] g_main_run(0x2e1280, 0x2e1280, 0x1, 0xfd414538, 0xfd41453c, 0x11bbec), at
0xfe7b7f64
dbx: warning: can't find file
"/home/gisburn/package-builds/gtk+/gtk+-1.2.8/objdir/gtk/gtkmain.lo"
  [19] gtk_main(0xc68b0, 0xc84b0, 0xffbef0d4, 0xffbef0d8, 0x0, 0xfd3a6b0c), at
0xfe9d60a0
  [20] nsAppShell::Run(this = ???) (optimized), at 0xfd3a6a60 (line ~332) in
"nsAppShell.cpp"
  [21] nsAppShellService::Run(this = ???) (optimized), at 0xfd4aed6c (line ~451)
in "nsAppShellService.cpp"
  [22] main1(argc = ???, argv = ???, nativeApp = ???) (optimized), at 0x1a2c0
(line ~1518) in "nsAppRunner.cpp"
  [23] main(argc = ???, argv = ???) (optimized), at 0x1aca8 (line ~1878) in
"nsAppRunner.cpp"
-- snip --

Comment 7

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

I don't understand. This patch doesn't actually prevent a crash if aSrc or aDst
is null, right?

Comment 8

[Roland Mainz]

Robert O'Callahan wrote:
> I don't understand. This patch doesn't actually prevent a crash if aSrc or 
> aDst is null, right?

Uhm, the patch prevents the crash.
If either |aSrc| or |aDst| are |nsnull| a |NS_ERROR_NULL_POINTER| will be
returned. The remaining parts in the patch cleanup the lazy error checking in
that function to avoid any future trouble in this piece of code.

Comment 9

[Roland Mainz]

roc:
Can you give me an r= , please ?

Comment 10

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

Comment on attachment 94240 [details] [diff] [review]
Patch for 2002-08-04-08-trunk

r=roc+moz

You're right. Sorry about that.

Comment 11

[Roland Mainz]

Requesting sr= ...

Comment 12

[Boris Zbarsky [:bzbarsky]]

> +  NS_ENSURE_TRUE(aSrc != nsnull, NS_ERROR_NULL_POINTER);
> +  NS_ENSURE_TRUE(aDst != nsnull, NS_ERROR_NULL_POINTER);

NS_ENSURE_ARG_POINTER(aSrc);
NS_ENSURE_ARG_POINTER(aDst);

Is there any normal mode of operation under which aSrc or aDst would be null? 
If so, please comment here saying what it is. If not, add an assertion to that
effect so that the callers can be found and fixed.

The patch changes the functionality in the case when the second source cannot be
locked or if the bitmap formats do not match.  In those cases, no blending will
happen at all, unlike in the existing code.  Is this intentional?

Comment 13

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

> Is there any normal mode of operation under which aSrc or aDst would be null?

No.

> In those cases, no blending will happen at all

If that happens you're pretty much screwed so it doesn't matter what we do as
long as we don't crash :-).

Comment 14

[Roland Mainz]

Boris Zbarsky wrote:
> > +  NS_ENSURE_TRUE(aSrc != nsnull, NS_ERROR_NULL_POINTER);
> > +  NS_ENSURE_TRUE(aDst != nsnull, NS_ERROR_NULL_POINTER);
> 
> NS_ENSURE_ARG_POINTER(aSrc);
> NS_ENSURE_ARG_POINTER(aDst);

Erm... |NS_ENSURE_ARG_POINTER| is defined as
-- snip --
333 #define NS_ENSURE_ARG_POINTER(arg)                            \
334   NS_ENSURE_TRUE(arg, NS_ERROR_INVALID_POINTER)
-- snip --
What's better here - return |NS_ERROR_INVALID_POINTER| or
|NS_ERROR_NULL_POINTER| ?

> Is there any normal mode of operation under which aSrc or aDst would be null? 

AFAIK no.

> The patch changes the functionality in the case when the second source cannot
> be locked or if the bitmap formats do not match.  In those cases, no blending 
> will happen at all, unlike in the existing code.  Is this intentional?

Yes, the original code isn't right here. If we fail to lock something then we
should return that error instead of trying something wrong and return success.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> What's better here - return |NS_ERROR_INVALID_POINTER| or
> |NS_ERROR_NULL_POINTER| ?

It's better to use a more descriptive macro.  Which can then be fixed as
needed... (why do we even have both of those errors?  sigh.)

Change that and add the assertion like I asked for on aDst and aSrc and sr=bzbarsky

Comment 16

[Roland Mainz]

Attachment: Patch for 2002-08-30-08-trunk per bz's (superreview) comments

Additionally I converted the manual refcounting to nsCOMPtr usage in this patch
per discussion with bz...

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 97509 [details] [diff] [review]
Patch for 2002-08-30-08-trunk per bz's (superreview) comments

+  NS_ASSERTION(aSrc != nsnull, "nsBlender::Blend() called with nsnull aSrc");
+  NS_ASSERTION(aDst != nsnull, "nsBlender::Blend() called with nsnull aDst");

No need for the "!= nsnull" parts there.

sr=bzbarsky with that change

Comment 18

[Roland Mainz]

Attachment: New patch for 2002-08-30-08-trunk per bz's comment

Comment 19

[Roland Mainz]

Comment on attachment 97740 [details] [diff] [review]
New patch for 2002-08-30-08-trunk per bz's comment

Carrying over r=/sr= per bz's permission via IRC:
-- snip --
*bz* sr=me; carry the stuff over.
-- snip --

Comment 20

[mjudge]

adding to CC

Comment 21

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

Roland, are you going to check this in?

Comment 22

[Roland Mainz]

Robert O'Callahan wrote:
> Roland, are you going to check this in?

I've asked mjudge to do it ...

Comment 23

[mjudge]

checked it into trunk marking as fixed now