Last Comment Bug 593744 - nsWindow.cpp should assert if mLayerManager is null before Returning from nsWindow::GetLayerManager()
: nsWindow.cpp should assert if mLayerManager is null before Returning from nsW...
[good first bug]
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Windows XP
-- normal (vote)
: mozilla10
Assigned To: Marco Túlio Costa
: Milan Sreckovic [:milan]
Depends on: 593521
  Show dependency treegraph
Reported: 2010-09-05 21:27 PDT by Justin Wood (:Callek) [away until Feb 27]
Modified: 2011-10-03 08:02 PDT (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Added the requested assert (907 bytes, patch)
2011-08-03 20:47 PDT, Marco Túlio Costa
joe: review+
bugspam.Callek: feedback+
Details | Diff | Splinter Review

Description User image Justin Wood (:Callek) [away until Feb 27] 2010-09-05 21:27:14 PDT
From an IRC discussion, following

mLayerManager can be returned null in (currently unsupported) cases. I suggested we assert that it is not null on a return here, since I was also told it is expected not to be. (Before this cset we never returned null here).

The assert will help prevent future mistakes that can happen much easier given the web of ifdefs inside this function.
Comment 1 User image Marco Túlio Costa 2011-08-03 20:47:23 PDT
Created attachment 550597 [details] [diff] [review]
Added the requested assert
Comment 2 User image Justin Wood (:Callek) [away until Feb 27] 2011-08-03 21:10:53 PDT
Comment on attachment 550597 [details] [diff] [review]
Added the requested assert

I'm not a reviewer here, and it looks like the code has changed significantly since I filed this. I wasn't able to detangle it at first glance, but it does look like we still use |new| to create a LayerManager in this fallback (even though the fallback is no longer #ifdef'ed out in some cases).

What I am not sure about is if this is an infallible alloc or not in this part of the code.

The actual addition of the assert is technically correct, over to Joe for the concept on if this assert is actually useful here now.
Comment 3 User image Joe Drew (not getting mail) 2011-08-04 14:32:42 PDT
new is always infallible in Mozilla code; you have to use |new (fallible_t())| to get a fallible new.
Comment 4 User image Justin Wood (:Callek) [away until Feb 27] 2011-10-01 17:38:22 PDT
Marco, I'm sorry we didn't catch this before now. Typically once you have review you would add checkin-needed so someone can get around to actually pushing this live. (I'm hoping the patch has not bitrotted since review)
Comment 6 User image Marco Bonardo [::mak] 2011-10-03 08:02:39 PDT

Note You need to log in before you can comment on or make changes to this bug.