373305 - static InlineBackgroundData instance containing nsRect in layout triggering runtime static ctor/dtor warnings

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[timeless]

[XPCOM objects created/destroyed from static ctor/dtor] (gActivityTLS != BAD_TLS_INDEX && NS_PTR_TO_INT32(PR_Get ThreadPrivate(gActivityTLS)) == 0) 

gkgfx!nsRect::nsRect
gklayout!InlineBackgroundData::InlineBackgroundData
gklayout!`dynamic initializer for 'gInlineBGData''(void)+0xd

static InlineBackgroundData gInlineBGData; (nsCSSRendering.cpp)

Comment 1

[Boris Zbarsky [:bzbarsky]]

We can allocate/deallocate this in the module ctor/dtor, if it doesn't go away entirely with cairo-ization of nsCSSRendering.

Comment 2

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

It won't go away unless we regress what it does.

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK.  So we should add static Init and Destroy or some such to nsCSSRendering.  Sound ok?

Comment 4

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

Sure.

Comment 5

[Josh Aas]

Attachment: fix v1.0

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 258507 [details] [diff] [review]
fix v1.0

>Index: base/nsCSSRendering.cpp

>+nsresult nsCSSRendering::Initialize()
>+{
>+  if (!gInlineBGData)
>+    gInlineBGData = new InlineBackgroundData;
>+
>+  return NS_OK;
>+}

If this always returns NS_OK, why not make it void -- except that allocation can fail, so you should check for OOM here.

>+nsresult nsCSSRendering::Destroy()
>+{
>+  if (gInlineBGData) {
>+    delete gInlineBGData;
>+    gInlineBGData = nsnull;
>+  }
>+
>+  return NS_OK;
>+}

This can't fail -- just make it void.

Comment 7

[Jeff Walden [:Waldo]]

(In reply to comment #6)
> >+nsresult nsCSSRendering::Destroy()
> >+{
> >+  if (gInlineBGData) {
> >+    delete gInlineBGData;
> >+    gInlineBGData = nsnull;
> >+  }
> >+
> >+  return NS_OK;
> >+}

Also, since delete is null-safe, you could remove the if if you wanted.

Comment 8

[Josh Aas]

As for return values, I thought about doing void but I followed the example set in another situation.

Comment 9

[Josh Aas]

Attachment: fix v1.1

Comment 10

[Boris Zbarsky [:bzbarsky]]

Put this inside nsLayoutStatics?  That's where all this sort of thing lives.

Comment 11

[Josh Aas]

bz - put what inside of nsLayoutStatistics?

Comment 12

[Josh Aas]

nm, I get it. patch coming up.

Comment 13

[Josh Aas]

Attachment: fix v1.2

Comment 14

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

Comment on attachment 258572 [details] [diff] [review]
fix v1.2

>+nsresult nsCSSRendering::Init()
>+{  
>+  if (!gInlineBGData)
>+    gInlineBGData = new InlineBackgroundData;

Change this to

  NS_ASSERTION(!gInlineBGData, "Init called twice");
  gInlineBGData = new InlineBackgroundData();

and r+sr=dbaron.

Comment 15

[Josh Aas]

fixed on trunk