Allocate heap nsFrameLists from the pres shell arena

RESOLVED FIXED in mozilla22

Status

()

enhancement
P5
normal
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

({sec-want})

unspecified
mozilla22
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

Posted patch exploration (obsolete) — Splinter Review
I'm not sure if this is worth doing for the relatively few heap
nsFrameLists that we have.

pros:
1. pres arena LRU cache (is it really better than jemalloc though?)
2. avoids freeing memory for each one separately when destroying the shell
   (many of these lists are transient and don't live that long though)
3. leak detection by default (when the pres shell is destroyed)

cons:
1. slightly more heavy handed to allocate/destroy since you have to
   provide the pres shell

PresArena also gives us poisoning but since the two frame members of
nsFrameList are already poisoned I don't think it adds much value.

Also, the FramePropertyTable changes in the patch to support a destructor
is probably overkill, we could just free these properties manually in the
respective frame that owns them.

What do you think, is it worth doing this?
Don't use DestroyOverflowList() to destroy the overflow list in
nsContainerFrame::DestroyFrom -- use SafelyDestroyFrameListProp instead,
because it's more robust (bug 606642) and it makes the next patch possible.
Attachment #730922 - Flags: review?(bzbarsky)
Specifically, this wasn't done for the OutsideBullet list which was
destroyed by the property destructor, potentially after the entire
frame tree was gone.  Now we'll assert that the property destructor
isn't called.

Use SafelyDestroyFrameListProp when destroying PushedFloat and
OverflowOutOfFlows frame lists since it's more robust.

Destroy [Excess]OverflowContainers frame lists if they exist, regardless
of the IsFrameOfType(nsIFrame::eCanContainOverflowContainers) bit
(since the oveflow continuation tracker doesn't check that before
creating these lists -- this was the source of crash bugs before
that bit was added to ColumnSetFrame).

Assert in SetPropTableFrames that the property doesn't exist, because
if it does the property destructor will run.
Attachment #730927 - Flags: review?(bzbarsky)
The main patch, which is fairly straight-forward now:

"new nsFrameList()" becomes "new (shell) nsFrameList()".
"delete list" becomes "if (list) list->Delete(shell)" - note also that
an additional assertion was added that list is empty when deleted.

"nsAutoPtr<nsFrameList> list(StealSomeFrames())" becomes 
"AutoFrameListPtr list(aPresContext, StealSomeFrames())"
Attachment #599601 - Attachment is obsolete: true
Attachment #730930 - Flags: review?(bzbarsky)
In summary, the main changes are:
1. allocate (heap) nsFrameLists from the shell arena
2. frame property values that are nsFrameLists are destroyed by the
   frame that owns them, in DestroyFrom, thus the property destructor
   function is never called (this was actually already the case except
   for the OutsideBullet list)

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=313fcb091ba9
Comment on attachment 730922 [details] [diff] [review]
Use SafelyDestroyFrameListProp also for the OverflowList

r=me
Attachment #730922 - Flags: review?(bzbarsky) → review+
Comment on attachment 730923 [details] [diff] [review]
Simplify DestroyOverflowList() by requiring that the list is empty

r=me
Attachment #730923 - Flags: review?(bzbarsky) → review+
Comment on attachment 730925 [details] [diff] [review]
Allocate nsFrameList::sEmptyList from the .rodata segment, not the heap

The assertion messages still mention sEmptyList.  Fix that?

>+  return *(nsFrameList*)&mozilla::layout::detail::gEmptyFrameListBytes;

reinterpret_cast.

r=me
Attachment #730925 - Flags: review?(bzbarsky) → review+
Comment on attachment 730927 [details] [diff] [review]
Make frames destroy all child frames and remove their nsFrameList properties when destroyed

Hmm.  A bit worried about the perf impact of regetting the prop repeatedly for overflow out-of-flows, but r=me....
Attachment #730927 - Flags: review?(bzbarsky) → review+
Comment on attachment 730930 [details] [diff] [review]
Allocate heap nsFrameLists from the shell arena

>+  void operator delete(void*) MOZ_DELETE;

Shouldn't this have a size_t argument too?

r=me either way, as long as deleting a framelist via delete doesn't compile.
Attachment #730930 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #13)
> The assertion messages still mention sEmptyList.  Fix that?

fixed

> reinterpret_cast.

fixed 

(In reply to Boris Zbarsky (:bz) from comment #14)
> Hmm.  A bit worried about the perf impact of regetting the prop repeatedly
> for overflow out-of-flows, but r=me....

Yeah, it's a temporary compromise.  I want to create a frame bit for the
remaining few lists that don't have it, and then gate all lookups on it.
I figured it would help to make frame list handling more uniform.
Filed bug 856630 about it.

(In reply to Boris Zbarsky (:bz) from comment #15)
> >+  void operator delete(void*) MOZ_DELETE;
> 
> Shouldn't this have a size_t argument too?

Either way is fine.  The C++11 Standard says that 'operator delete(void*)'
is the (non-placement) deallocator if it exists, otherwise it's
'operator delete(void*, size_t)' if it exists.

I checked that it fails to compile if used.
Keywords: sec-want
You need to log in before you can comment on or make changes to this bug.