Closed Bug 947022 Opened 7 years ago Closed 7 years ago

statically type docshell stuff for nsIPresShell and nsPresContext

Categories

(Core :: DOM: Navigation, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Details

(Whiteboard: [qa-])

Attachments

(1 file)

No description provided.
Trevor, how did you audit to make sure you got all the callers?  In particular, I'm worried about cases where a caller used to get an addrefed pointer but now gets a raw pointer...
(In reply to Boris Zbarsky [:bz] from comment #2)
> Trevor, how did you audit to make sure you got all the callers?  In
> particular, I'm worried about cases where a caller used to get an addrefed
> pointer but now gets a raw pointer...


Well, this used to return an already_AddRefed not a raw pointer so you'd have to cast the return  value, or do something pretty strange to have something dangerous happen.  I also grepped for all the places that called GetContainer to see which could use GetDocShell() in the process of doing that I think I saw all the callers and non did anything funny with raw pointers they all just assigned to a nsCOMPtr which is still fine.  I think there aren't too many callers left so renaming it to double check shouldn't be too bad if you'd like.
> so you'd have to cast the return  value, or do something pretty strange to have
> something dangerous happen. 

Something as simple as:

  already_AddRefed(foo->GetContainer())

would change behavior pretty significantly.  And the failure mode is pretty unpleasant.  Thank God we no longer allow returning a raw pointer to auto-construct an already_AddRefed, at least...

If you're willing to do the renaming test, I would very much appreciate it!  I know it's a pain, but I'd rather not end up with deleted-object references.
Comment on attachment 8343432 [details] [diff] [review]
bug 947022 - type nsIPresShell::mForwardingContainer and nsPresContext::mContainer

>+++ b/layout/base/nsCSSFrameConstructor.cpp
> nsCSSFrameConstructor::FindXULMenubarData(Element* aElement,
>+  nsCOMPtr<nsIDocShell> treeItem =
>     aStyleContext->PresContext()->GetContainer();

That can't possibly compile...  Should be GetDocShell.

I think we should just rename GetContainer() to GetContainerWeak() so binary extensions won't get screwed over too.

r=me with those done
Attachment #8343432 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/4c67b63a52c2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.