1230034 - Make frame property typesafe

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

Currently frame properties are not typesafe. They use void pointer and cast to set and get data. It should be made typesafe.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27238091c35f

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bc9716a99b4

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=de339905f2f2

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1230034 part 1 - Remove NS_PROPERTY_DESCRIPTOR_CONST macro.

Bug 1230034 part 1 - Remove NS_PROPERTY_DESCRIPTOR_CONST macro.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1230034 part 2 - Constify aFrame parameters of frame property functions.

Bug 1230034 part 2 - Constify aFrame parameters of frame property functions.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1230034 part 3 - Move some frame property declaration around.

Bug 1230034 part 3 - Move some frame property declaration around.

Bidi-related frame properties are moved from nsIFrame.h to nsBidi.h because their type, nsBidiLevel, is defined in nsBidi.h. Comparing to include nsBidi.h in nsIFrame.h, it seems far cheaper to include nsIFrame.h in nsBidi.h instead.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1230034 part 4 - Make FramePropertyDescriptor to be a template.

Bug 1230034 part 4 - Make FramePropertyDescriptor to be a template.

This patch makes methods of FramePropertyTable and FrameProperties to be simple template wrapper functions. Then it converts all references to FramePropertyDescriptor to use "void" parameter to simulate the current unsafe behavior.

Changes outside FramePropertyTable and nsIFrame are mostly temporary changes (especially the ones just add a "<>" to FramePropertyDescriptor), and they are likely to be further changed in following commits. Those changes are here just for making compiler happy.

SmallValueHolder added in this commit will be used in part 6.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1230034 part 5 - Convert all frame properties which use DeleteValue and ReleaseValue as destructor to be typesafe.

Bug 1230034 part 5 - Convert all frame properties which use DeleteValue and ReleaseValue as destructor to be typesafe.

By changing signature of those two functions, we make compiler complain about
all their existing uses, so we can find all of them and convert them.

Some of the callsites of Get() with those properties are also converted, but not
all of them. It is fine because if there is any incorrect conversion, compilers
is able to find out now. So they are completely typesafe.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1230034 part 6 - Convert all frame properties which do not hold pointer to be typed.

Bug 1230034 part 6 - Convert all frame properties which do not hold pointer to be typed.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1230034 part 7 - Convert nsIFrame::GenConProperty to be typed.

Bug 1230034 part 7 - Convert nsIFrame::GenConProperty to be typed.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1230034 part 8 - Convert frame properties which assert on destructor to be typed.

Bug 1230034 part 8 - Convert frame properties which assert on destructor to be typed.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1230034 part 9 - Convert FrameLayerBuilder::LayerManagerDataProperty to be typed.

Bug 1230034 part 9 - Convert FrameLayerBuilder::LayerManagerDataProperty to be typed.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1230034 part 10 - Convert remaining frame properties to by typed and remove the unsafe declaring macro.

Bug 1230034 part 10 - Convert remaining frame properties to be typed and remove the unsafe declaring macro.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8695745 [details]
MozReview Request: Bug 1230034 part 4 - Make FramePropertyDescriptor to be a template.

froydnj, could you please have a look at the changes to FramePropertyTable.h in this commit? I'm not completely confident I'm doing the right thing there.

Comment 15

[Nathan Froyd [:froydnj]]

Comment on attachment 8695745 [details]
MozReview Request: Bug 1230034 part 4 - Make FramePropertyDescriptor to be a template.

Apologies for the delay.

I think this looks reasonable; I skimmed some of the patches and they look rather nice with this change.

I have two comments.

The first is a question about whether this does anything unreasonable to codesize, since we now have a bunch of type-specialized things all over the place.  I don't *think* it does, but I haven't examined the patches in detail.  It'd be good to verify that.

The second is out-loud-wondering: UniquePtr uses mozilla::Pair under the hood to make sizeof(UniquePtr<T>) == sizeof(T*) in the common case and avoid storing a function pointer for the deleter.  This also means that we don't have to call |delete| or something through a function pointer in UniquePtr's destructor, but the compiler will call |delete| directly for us.  I was wondering whether we could do the same thing here...but I think we can't do the same thing here, because we don't propagate type information all the way through FramePropertyTable, right?

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Nathan Froyd [:froydnj] from comment #15)
> Comment on attachment 8695745 [details]
> MozReview Request: Bug 1230034 part 4 - Make FramePropertyDescriptor to be a
> template.
> 
> Apologies for the delay.
> 
> I think this looks reasonable; I skimmed some of the patches and they look
> rather nice with this change.

So basically you think it is fine to cast the functions and pointers in this way?

> I have two comments.
> 
> The first is a question about whether this does anything unreasonable to
> codesize, since we now have a bunch of type-specialized things all over the
> place.  I don't *think* it does, but I haven't examined the patches in
> detail.  It'd be good to verify that.

I made them a simple wrap, and expect they are inlined by the compiler so it shouldn't affect the codesize I suppose. But yeah, it would be good to verify.

> The second is out-loud-wondering: UniquePtr uses mozilla::Pair under the
> hood to make sizeof(UniquePtr<T>) == sizeof(T*) in the common case and avoid
> storing a function pointer for the deleter.  This also means that we don't
> have to call |delete| or something through a function pointer in UniquePtr's
> destructor, but the compiler will call |delete| directly for us.  I was
> wondering whether we could do the same thing here...but I think we can't do
> the same thing here, because we don't propagate type information all the way
> through FramePropertyTable, right?

Yeah, since the frame property table stores different types of data in one hash table, so I think it is just impossible to keep any type information there. I cast everything to void* in the wrapper functions.

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to Xidorn Quan [:xidorn] (UTC-5) from comment #16)
> (In reply to Nathan Froyd [:froydnj] from comment #15)
> > Comment on attachment 8695745 [details]
> > MozReview Request: Bug 1230034 part 4 - Make FramePropertyDescriptor to be a
> > template.
> > 
> > Apologies for the delay.
> > 
> > I think this looks reasonable; I skimmed some of the patches and they look
> > rather nice with this change.
> 
> So basically you think it is fine to cast the functions and pointers in this
> way?

I'm not sure what you mean by this.  Do you have a specific point in the code for this sort of thing?

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

I mean the changes in FramePropertyTable.h, especially in class FramePropertyTable, especially FramePropertyTable::Cast.

Comment 19

[Nathan Froyd [:froydnj]]

https://reviewboard.mozilla.org/r/27179/#review24809

::: layout/base/FramePropertyTable.h:195
(Diff revision 1)
> +    static_assert(
> +      sizeof(FramePropertyDescriptor<T>) == sizeof(FramePropertyDescriptor<>) &&
> +      (offsetof(FramePropertyDescriptor<T>, mDestructor) ==
> +       offsetof(FramePropertyDescriptor<>, mDestructor)) &&
> +      (offsetof(FramePropertyDescriptor<T>, mDestructorWithFrame) ==
> +       offsetof(FramePropertyDescriptor<>, mDestructorWithFrame)),
> +      "FramePropertyDescriptor<T> must have the same layout with "
> +      "FramePropertyDescriptor<>, otherwise the behavior would be "
> +      "truly unpredictable!");

I have a slight preference for splitting these up, but this isn't my code.

I applaud your paranoia here.  I think a better sequence of checks would be:

static_assert(mDestructor offsets are equal)
static_assert(mDestructorWithFrame offsets are equal)
static_assert(sizeof(mDestructor) + sizeof(mDestructorWithFrame) == sizeof(FramePropertyDescriptor<>))
static_assert(sizeof(mDestructor) + sizeof(mDestructorWithFrame) == sizeof(FramePropertyDescriptor<T>))

The last check(s) is intended to ensure that somebody has to update these checks if they add another field (not perfect, due to potential struct padding and whatnot).  Alternatively, I guess we could say:

static const sDescriptorSize = sizeof(mDestructor) + sizeof(mDestructorWithFrame);
static_assert(sDescriptorSize == sizeof(FramePropertyDescriptor<>));
static_assert(sDescriptorSize == sizeof(FramePropertyDescriptor<T>));

::: layout/base/FramePropertyTable.h:211
(Diff revision 1)
> +  template<typename T>
> +  union ReinterpretHelper

What is the benefit of this relative to simply using reinterpret_cast?  Is T ever a function type?

Oh, this is because we sometimes have U\* and reinterpret_cast would complain about reinterpret_cast<U\*>(void\* thing) (or vice versa?)?  Might be worth a comment.

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

https://reviewboard.mozilla.org/r/27179/#review24809

> What is the benefit of this relative to simply using reinterpret_cast?  Is T ever a function type?
> 
> Oh, this is because we sometimes have U\* and reinterpret_cast would complain about reinterpret_cast<U\*>(void\* thing) (or vice versa?)?  Might be worth a comment.

This is for smaller types like int32_t or even bool/uint8_t. (See SmallValueHolder and FramePropertyTypeHelper in this file) I considered using reinterpret_cast, but I could not find any reference states that it is safe to reinterpret_cast between two types with different size, so I guess it is safer to use union here.

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

https://reviewboard.mozilla.org/r/27179/#review24809

> This is for smaller types like int32_t or even bool/uint8_t. (See SmallValueHolder and FramePropertyTypeHelper in this file) I considered using reinterpret_cast, but I could not find any reference states that it is safe to reinterpret_cast between two types with different size, so I guess it is safer to use union here.

Also there are some places store float in frame property, and it seems to me we probably have to use union anyway for float.

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8695745 [details]
MozReview Request: Bug 1230034 part 4 - Make FramePropertyDescriptor to be a template.

A friend told me that casting function to a different type then calling it is explicitly specified as an undefined behavior in the C++ spec, and it is possible to workaround this limitation. So cancel review for now, and I'll push a new set soon.

(I probably shouldn't use MozReview for this bug as there are too many patches and MozReview will again produce too many rubbishes :(

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8d234e36db2

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=25b0cb8ae8a9

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0aa96c3dfbb6

Comment 26

[Xidorn Quan [:xidorn] UTC+10]

Hmmm, some bustage from m-c in try push in comment 24...

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

About binary size consideration, I compared the build between
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4f4483b2ff8 (before the patchset) and
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c21b8e37e5e8 (after the patchset)

The size difference on xul library on optimized builds are:
+---------------+-------------+------------+--------+---------+
|     Build     | Size Before | Size After |  Diff  |    %    |
+---------------+-------------+------------+--------+---------+
|  Linux 32bit  |   93,775 KB |  93,782 KB | + 7 KB | ~0.007% |
|  Linux 64bit  |   99,537 KB |  99,539 KB | + 2 KB | ~0.002% |
|   Mac OS X    |  300,237 KB | 300,254 KB | +17 KB | ~0.006% |
| Windows 32bit |   37,696 KB |  37,700 KB | + 4 KB | ~0.011% |
| Windows 64bit |   49,426 KB |  49,440 KB | +14 KB | ~0.028% |
+---------------+-------------+------------+--------+---------+

Does this change looks reasonable? It seems fine, although several KBs change still looks weird to me. I'd expect the size doesn't change at all...

Comment 28

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1230034 part 1 - Remove NS_PROPERTY_DESCRIPTOR_CONST macro.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27173/diff/1-2/

Comment 29

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1230034 part 2 - Constify aFrame parameters of frame property functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27175/diff/1-2/

Comment 30

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1230034 part 3 - Move some frame property declaration around.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27177/diff/1-2/

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1230034 part 4 - Make FramePropertyDescriptor to be a template.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27179/diff/1-2/

Comment 32

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1230034 part 5 - Convert all frame properties which use DeleteValue and ReleaseValue as destructor to be typesafe.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27181/diff/1-2/

Comment 33

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1230034 part 6 - Convert all frame properties which do not hold pointer to be typed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27183/diff/1-2/

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1230034 part 7 - Convert nsIFrame::GenConProperty to be typed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27185/diff/1-2/

Comment 35

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1230034 part 8 - Convert frame properties which assert on destructor to be typed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27187/diff/1-2/

Comment 36

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1230034 part 9 - Convert FrameLayerBuilder::LayerManagerDataProperty to be typed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27189/diff/1-2/

Comment 37

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1230034 part 10 - Convert remaining frame properties to by typed and remove the unsafe declaring macro.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27191/diff/1-2/

Comment 38

[Xidorn Quan [:xidorn] UTC+10]

I don't know what's wrong with MozReview... It doesn't reuse the old attachments...

Comment 39

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8698806 [details]
MozReview Request: Bug 1230034 part 4 - Make FramePropertyDescriptor to be a template.

This should eliminate any undefined behavior. There is even no "cast" used :)

froydnj, if you could check the code again, it would be really appreciated. The new content of "struct FramePropertyDescriptor" is the part I feel least confident.

Comment 40

[Xidorn Quan [:xidorn] UTC+10]

dbaron, FWIW, I believe all patches in the patchset except part 4 should be easy to review.

Comment 41

[Xidorn Quan [:xidorn] UTC+10]

https://reviewboard.mozilla.org/r/27175/#review25149

::: layout/base/FramePropertyTable.h:221
(Diff revision 2)
>      : mTable(aTable), mFrame(const_cast<nsIFrame*>(aFrame)) {}

In local code, mFrame here has been changed to const as well, and the constructor contains non-const nsIFrame* parameter has been removed.

Comment 42

[Nathan Froyd [:froydnj]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #27)
> About binary size consideration, I compared the build between
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4f4483b2ff8 (before
> the patchset) and
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c21b8e37e5e8 (after
> the patchset)
> 
> The size difference on xul library on optimized builds are:
> +---------------+-------------+------------+--------+---------+
> |     Build     | Size Before | Size After |  Diff  |    %    |
> +---------------+-------------+------------+--------+---------+
> |  Linux 32bit  |   93,775 KB |  93,782 KB | + 7 KB | ~0.007% |
> |  Linux 64bit  |   99,537 KB |  99,539 KB | + 2 KB | ~0.002% |
> |   Mac OS X    |  300,237 KB | 300,254 KB | +17 KB | ~0.006% |
> | Windows 32bit |   37,696 KB |  37,700 KB | + 4 KB | ~0.011% |
> | Windows 64bit |   49,426 KB |  49,440 KB | +14 KB | ~0.028% |
> +---------------+-------------+------------+--------+---------+
> 
> Does this change looks reasonable? It seems fine, although several KBs
> change still looks weird to me. I'd expect the size doesn't change at all...

Is that comparing just the sizes of libxul on disk or comparing the actual numbers as returned by something like size(1) on Linux (which measures the bits that actual get loaded during execution)?

Comment 43

[Xidorn Quan [:xidorn] UTC+10]

Just measured the sizes of libxul files. I didn't know anything about size(1).

Comment 44

[Xidorn Quan [:xidorn] UTC+10]

Result from size(1):


Linux 32bit:
--------------------------------------------
           text     data     bss      dec
(before) 65586894  2912932  639140  69138966
( after) 65588742  2912900  639332  69140974
--------------------------------------------
            +1848      +32    +192     +1908

Linux 64bit:
--------------------------------------------
           text     data     bss      dec
(before) 65607627  5533284  774344  71915255
( after) 65608147  5533348  774728  71916223
--------------------------------------------
            +520       +64    +384      +968

Mac OS X (x86):
--------------------------------------------
           text     data     bss      dec
(before) 80006250  2988540  622036  83616826
( after) 80006410  2988524  622036  83616970
--------------------------------------------
             +160      -16      +0      +144

Mac OS X (x64):
--------------------------------------------
           text     data     bss      dec
(before) 88696345  5581168  753140  95030653
( after) 88696689  5581168  753140  95030997
--------------------------------------------
             +344       +0      +0      +344

Windows 32bit:
----------------------------------
           text    data     dec
(before) 37978943 616450  38595393
( after) 37983147 615938  38599085
----------------------------------
            +4204   -512     +3692

Windows 64bit:
----------------------------------
           text    data     dec
(before) 49673809 932866  50606675
( after) 49688157 932354  50620511
----------------------------------
           +14348   -512    +13836

Comment 45

[Nathan Froyd [:froydnj]]

Those numbers seem reasonable enough, though I wonder what the increases are coming from, particularly on Windows.  Newly-introduced functions the compiler can't merge together or something like that?

Comment 46

[Nathan Froyd [:froydnj]]

Comment on attachment 8695745 [details]
MozReview Request: Bug 1230034 part 4 - Make FramePropertyDescriptor to be a template.

https://reviewboard.mozilla.org/r/27179/#review25551

Sorry for the long delay in feedback.  I think this is a bit nicer.

::: layout/base/FramePropertyTable.h:95
(Diff revision 2)
> +    Dtor(reinterpret_cast<T*>(aPropertyValue));

I think static_cast from void pointers is sufficient.

::: layout/base/FramePropertyTable.h:101
(Diff revision 2)
> +    Dtor(aFrame, reinterpret_cast<T*>(aPropertyValue));

Likewise here.

::: layout/base/FramePropertyTable.h:110
(Diff revision 2)
> +class SmallValueHolder;

SmallValue might be descriptive here, but it's not very descriptive in other contexts.  PointerSizeOrSmallerValue is quite long, but accurate.  (This is really more dbaron's realm, though, so he can make the final call.)

::: layout/base/FramePropertyTable.h:171
(Diff revision 2)
>     * Get a property value for a frame. This requires one hashtable
>     * lookup (using the frame as the key) and a linear search through
>     * the properties of that frame. If the frame has no such property,
>     * returns null.
>     * @param aFoundResult if non-null, receives a value 'true' iff
>     * the frame has a value for the property. This lets callers
>     * disambiguate a null result, which can mean 'no such property' or
>     * 'property value is null'.

The documentation for Get and Remove will need to be updated, as they can no longer return null for some types.  What is the right return-value for not-found small types, anyway?

Comment 47

[Nathan Froyd [:froydnj]]

Comment on attachment 8698806 [details]
MozReview Request: Bug 1230034 part 4 - Make FramePropertyDescriptor to be a template.

Sigh, MozReview.

Comment 48

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

Comment on attachment 8695742 [details]
MozReview Request: Bug 1230034 part 1 - Remove NS_PROPERTY_DESCRIPTOR_CONST macro.

https://reviewboard.mozilla.org/r/27173/#review26483

Comment 49

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

Comment on attachment 8695743 [details]
MozReview Request: Bug 1230034 part 2 - Constify aFrame parameters of frame property functions.

https://reviewboard.mozilla.org/r/27175/#review26485

Comment 50

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

Comment on attachment 8695744 [details]
MozReview Request: Bug 1230034 part 3 - Move some frame property declaration around.

https://reviewboard.mozilla.org/r/27177/#review26487

Comment 51

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

Comment on attachment 8695745 [details]
MozReview Request: Bug 1230034 part 4 - Make FramePropertyDescriptor to be a template.

https://reviewboard.mozilla.org/r/27179/#review29431

::: layout/base/FramePropertyTable.h:54
(Diff revision 2)
> +  MOZ_CONSTEXPR FramePropertyDescriptorUntyped(
> +    UntypedDestructor* aDtor, UntypedDestructorWithFrame* aDtorWithFrame)
> +    : mDestructor(aDtor)
> +    , mDestructorWithFrame(aDtorWithFrame)
> +  {}

Now that there's a constructor here, it would be a good idea to MOZ_ASSERT(!mDestructor || !mDestructorWithFrame, "only one destructor allowed")

::: layout/base/FramePropertyTable.h:110
(Diff revision 2)
> +class SmallValueHolder;

I don't really have an opinion here (on :froydnj's comment).

::: layout/base/FramePropertyTable.h:123
(Diff revision 2)
> +                "Type must be either integer or floating number");

"floating" -> "floating point"

::: layout/base/FramePropertyTable.h:166
(Diff revision 2)
> +    ReinterpretHelper<T> helper;
> +    helper.value = aValue;

Might it be better to set helper.ptr to nullptr before setting helper.value, so that if the value type is smaller than a pointer you don't end up storing uninitialized memory?  (Is that what will happen here if you don't do that?)

r=dbaron with those changes

Comment 52

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

Comment on attachment 8695746 [details]
MozReview Request: Bug 1230034 part 5 - Convert all frame properties which use DeleteValue and ReleaseValue as destructor to be typesafe.

https://reviewboard.mozilla.org/r/27181/#review29441

Comment 53

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

Comment on attachment 8695747 [details]
MozReview Request: Bug 1230034 part 6 - Convert all frame properties which do not hold pointer to be typed.

https://reviewboard.mozilla.org/r/27183/#review29443

Comment 54

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

Comment on attachment 8695748 [details]
MozReview Request: Bug 1230034 part 7 - Convert nsIFrame::GenConProperty to be typed.

https://reviewboard.mozilla.org/r/27185/#review29445

Comment 55

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

Comment on attachment 8695749 [details]
MozReview Request: Bug 1230034 part 8 - Convert frame properties which assert on destructor to be typed.

https://reviewboard.mozilla.org/r/27187/#review29447

::: layout/generic/nsContainerFrame.cpp:243
(Diff revision 2)
> -static void AppendIfNonempty(const nsIFrame* aFrame,
> +static void AppendIfNonempty(
> +  const nsIFrame* aFrame,
> -                            FramePropertyTable* aPropTable,
> +  FramePropertyTable* aPropTable,
> -                            const FramePropertyDescriptor<>* aProperty,
> +  nsContainerFrame::FrameListPropertyDescriptor aProperty,
> -                            nsTArray<nsIFrame::ChildList>* aLists,
> +  nsTArray<nsIFrame::ChildList>* aLists,
> -                            nsIFrame::ChildListID aListID)
> +  nsIFrame::ChildListID aListID)
>  {

Instead of this indentation change, could you follow the usual style by putting "static void" on its own line, and then lining up the parameters, to get:

static void
AppendIfNonempty(const nsIFrame* aFrame,
                 FramePropertyTable* ...

r=dbaron with that

Comment 56

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

Comment on attachment 8695750 [details]
MozReview Request: Bug 1230034 part 9 - Convert FrameLayerBuilder::LayerManagerDataProperty to be typed.

https://reviewboard.mozilla.org/r/27189/#review29449

::: layout/base/FrameLayerBuilder.cpp:1732
(Diff revision 2)
> -FrameLayerBuilder::RemoveFrameFromLayerManager(const nsIFrame* aFrame,
> -                                               void* aPropertyValue)
> +FrameLayerBuilder::RemoveFrameFromLayerManager(
> +  const nsIFrame* aFrame, nsTArray<DisplayItemData*>* aArray)

please keep normal indentation style here

Comment 57

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

Comment on attachment 8695751 [details]
MozReview Request: Bug 1230034 part 10 - Convert remaining frame properties to by typed and remove the unsafe declaring macro.

https://reviewboard.mozilla.org/r/27191/#review29453

Comment 58

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

Could you file a followup bug on getting rid of the remaining now-unneeded casts?

Comment 59

[Xidorn Quan [:xidorn] UTC+10]

Filed bug 1243559.

Comment 60

[Xidorn Quan [:xidorn] UTC+10]

https://reviewboard.mozilla.org/r/27179/#review29431

> Now that there's a constructor here, it would be a good idea to MOZ_ASSERT(!mDestructor || !mDestructorWithFrame, "only one destructor allowed")

The problem for adding MOZ_ASSERT is that, we would not be able to mark it constexpr, which could potentially add runtime burden even on non-debug build. (e.g. for dynamically initializing static local variables.)

I've made the constructor protected so that it is only accessible via its subclasses to avoid arbitrary number of destructor is passed in. I think this restriction is enough, and we do not need one more runtime check for it. We could probably add some comment here to make it more clear.

Comment 61

[Xidorn Quan [:xidorn] UTC+10]

https://reviewboard.mozilla.org/r/27179/#review25551

> SmallValue might be descriptive here, but it's not very descriptive in other contexts.  PointerSizeOrSmallerValue is quite long, but accurate.  (This is really more dbaron's realm, though, so he can make the final call.)

The "SmallValueHolder" itself would not commonly be used directly in the code. It is more likely to be wrapped inside the macro NS_DECLARE_FRAME_PROPERTY_SMALL_VALUE. If we call it NS_DECLARE_FRAME_PROPERTY_POINTER_SIZE_OR_SMALLER_VALUE, it would definitely be too long.

Since we have had a static_assert in ReinterpretHelper that the type must be smaller than a pointer, I'm not too worry about it being misused.

With the reasons above, and given dbaron doesn't really have opinion here, I'd prefer to just keep the current name.

Comment 62

[Xidorn Quan [:xidorn] UTC+10]

https://reviewboard.mozilla.org/r/27179/#review29431

> "floating" -> "floating point"

Actually now I'm not quite sure why did I want this assertion here... I guess I can just remove this restriction with no harm. Compilers would refuse to put any non-POD type in union ReinterpretHelper anyway. (It's true even with unrestricted unions support, which would require a non-trivial constructor and destructor if there is any non-POD member field.)

Comment 63

[Xidorn Quan [:xidorn] UTC+10]

https://reviewboard.mozilla.org/r/27179/#review29431

> Might it be better to set helper.ptr to nullptr before setting helper.value, so that if the value type is smaller than a pointer you don't end up storing uninitialized memory?  (Is that what will happen here if you don't do that?)

Hmmm, yeah, we should initialize the union first.

FWIW, It seems the better way is
> ReinterpretHelper<T> helper{};
which according to the C++ standard, should initialize the whole union to zero.

Although the spec I read is C++14, I believe it also applies to C++11. In addition, I tested all compilers we support, and it seems they all generate expected target code (vs. the code without the initializer).

Comment 64

[Xidorn Quan [:xidorn] UTC+10]

https://reviewboard.mozilla.org/r/27189/#review29449

> please keep normal indentation style here

But that would exceed 80 columns... Anyway, there is one even longer below, so I guess it's probably ok to exceed here...

Comment 65

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94d4c49590a052241f51572203504b0d5f8f4494
Bug 1230034 part 1 - Remove NS_PROPERTY_DESCRIPTOR_CONST macro. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6abdd91acbc65e9f0a818e1dec59c48a46d02f9
Bug 1230034 part 2 - Constify aFrame parameters of frame property functions. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/6af8cff960690de19591697de8f2578963908bc0
Bug 1230034 part 3 - Move some frame property declaration around. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b5384764e6cf1d8efd533f8513d511f9fae46eb
Bug 1230034 part 4 - Make FramePropertyDescriptor to be a template. r=froydnj,dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b4d8528c2f22908b546185439846b3e330305eb
Bug 1230034 part 5 - Convert all frame properties which use DeleteValue and ReleaseValue as destructor to be typesafe. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/d391b419c38b8331982c1b2725a01ef11442e868
Bug 1230034 part 6 - Convert all frame properties which do not hold pointer to be typed. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/38338ca5b3f933064f0af2b1e4bee76840335f64
Bug 1230034 part 7 - Convert nsIFrame::GenConProperty to be typed. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a5912b5ab4e519ca4deeaa853b49be42a0c3293
Bug 1230034 part 8 - Convert frame properties which assert on destructor to be typed. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f4cae60ca9baa765b4b8cf87af5bc1609e8778a
Bug 1230034 part 9 - Convert FrameLayerBuilder::LayerManagerDataProperty to be typed. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/33342c3d45efdb7610c9be83bf09d5664f898cd7
Bug 1230034 part 10 - Convert remaining frame properties to by typed and remove the unsafe declaring macro. r=dbaron

Comment 66

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/94d4c49590a0
https://hg.mozilla.org/mozilla-central/rev/c6abdd91acbc
https://hg.mozilla.org/mozilla-central/rev/6af8cff96069
https://hg.mozilla.org/mozilla-central/rev/2b5384764e6c
https://hg.mozilla.org/mozilla-central/rev/3b4d8528c2f2
https://hg.mozilla.org/mozilla-central/rev/d391b419c38b
https://hg.mozilla.org/mozilla-central/rev/38338ca5b3f9
https://hg.mozilla.org/mozilla-central/rev/7a5912b5ab4e
https://hg.mozilla.org/mozilla-central/rev/7f4cae60ca9b
https://hg.mozilla.org/mozilla-central/rev/33342c3d45ef

Comment 67

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/94d4c49590a0
https://hg.mozilla.org/mozilla-central/rev/c6abdd91acbc
https://hg.mozilla.org/mozilla-central/rev/6af8cff96069
https://hg.mozilla.org/mozilla-central/rev/2b5384764e6c
https://hg.mozilla.org/mozilla-central/rev/3b4d8528c2f2
https://hg.mozilla.org/mozilla-central/rev/d391b419c38b
https://hg.mozilla.org/mozilla-central/rev/38338ca5b3f9
https://hg.mozilla.org/mozilla-central/rev/7a5912b5ab4e
https://hg.mozilla.org/mozilla-central/rev/7f4cae60ca9b
https://hg.mozilla.org/mozilla-central/rev/33342c3d45ef

Comment 68

[Jonathan Kew [:jfkthame]]

Just FTR -- and in case anyone else runs into the same issue and comes looking -- this broke my local builds on Windows. I was getting persistent compile errors related to constexpr in nsIFrame.h, which I tracked down to the "part 5" patch here.

This was with Visual Studio (Community) 2015 and mozilla-build, etc.

Turns out the problem was that I had the original VS2015 release (which was all that was available when the machine was set up). After installing "Update 1", I can now build again.

Comment 69

[Jonathan Kew [:jfkthame]]

BTW, bug 1244092 describes the same failure I was seeing.