562833 - ARM alignment trap caused by misaligned structure in layout/base/nsDisplayList.cpp

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[junkmailnotread]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3
Build Identifier: fennec-1.1a2pre

fennec running on an HP iPAQ hx4700 (armv5tel) reported alignment traps after a few seconds of trivial use. For example this (from dmesg):

Alignment trap: fennec (1195) PC=0x40cb16ac Instr=0xe1c501d0 Address=0x4496c31c FSR 0x013

I traced this back to AddToItemGroup() in layout/base/nsDisplayList.cpp:

  if (aGroup->mEndItem == aItem &&
      (aGroup->mHasClipRect
       ? (aClipRect && aGroup->mClipRect == *aClipRect)
       : !aClipRect))  {
    aGroup->mEndItem = aItem->GetAbove();
    return aGroup;
  }

The expression "aGroup->mClipRect == *aClipRect" compares two structures of type gfxRect. These contain gfxFloat members. A gfxFloat is a double:

typedef double gfxFloat;

Consequently both *aGroup and *aClipRect should be aligned on 8-byte boundaries on the ARM architecture. However *aGroup was found to be aligned on a 4-byte boundary only.

*aGroup is of type nsDisplayList::ItemGroup, and as far as I could tell these were allocated by nsDisplayListBuilder::Allocate().

The alignment of memory allocated by nsDisplayListBuilder::Allocate() was set to "void*" (4 bytes) in nsDisplayListBuilder::nsDisplayListBuilder():

PL_InitArenaPool(&mPool, "displayListArena", 1024, sizeof(void*)-1);

Increasing this alignment to "double" (8 bytes) was found to fix the alignment traps:

diff -r bfc8c8a40286 layout/base/nsDisplayList.cpp
--- a/layout/base/nsDisplayList.cpp     Sat Apr 10 21:53:23 2010 -0500
+++ b/layout/base/nsDisplayList.cpp     Fri Apr 30 02:23:12 2010 +0100
@@ -78,7 +78,7 @@
       mAccurateVisibleRegions(PR_FALSE),
       mInTransform(PR_FALSE),
       mSyncDecodeImages(PR_FALSE) {
-  PL_InitArenaPool(&mPool, "displayListArena", 1024, sizeof(void*)-1);
+  PL_InitArenaPool(&mPool, "displayListArena", 1024, sizeof(double)-1);
 
   nsPresContext* pc = aReferenceFrame->PresContext();
   nsIPresShell *shell = pc->PresShell();

Obviously this fix doesn't necessarily apply to other architectures. Perhaps "void*" should be replaced by a union of all required base types instead.

Reproducible: Always

Comment 1

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

we should probably just pass '7' there explicitly.

Comment 2

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

Attachment: fix

Comment 3

[Boris Zbarsky [:bzbarsky]]

Are we fairly certain we'll never need more than 8-byte alignment?

Comment 4

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

That seems unlikely and anyway, can't we fix it then?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 442581 [details] [diff] [review]
fix

OK.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... or we could use the macro in the patch in bug 562844 :-)

Comment 7

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

I suppose we would make this NS_MAX(NS_ALIGNMENT_OF(void*), NS_ALIGNMENT_OF(double))?

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yeah, that would be the idea.

Comment 9

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

Alright, I'll make this depend on that.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That landed.  Care to revise?

Comment 11

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

Attachment: fix v2

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 454435 [details] [diff] [review]
fix v2

r=dbaron

Comment 13

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

http://hg.mozilla.org/mozilla-central/rev/9154d7221e0a