856243 - do_QueryFrame support is incomplete and thus a potential foot gun

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

Attachment: example

Consider the attached example and note that:
1. it compiles without a warning
2. any use of the result 'vp' is likely to be exploitable
3. as part of a real patch this sort of code is likely to get r+

The bug is that the ViewportFrame class should declare this:
  NS_DECL_QUERYFRAME_TARGET(ViewportFrame)
before it can be used as the target of a do_QueryFrame() call.
It compiles because ViewportFrame inherits nsContainerFrame which does
declare it.

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsQueryFrame.h

> 10  #define NS_DECL_QUERYFRAME_TARGET(classname)                    \
> 11    static const nsQueryFrame::FrameIID kFrameIID = nsQueryFrame::classname##_id;

> 59  class do_QueryFrame
> 60  {
> 61  public:
> 62    do_QueryFrame(nsQueryFrame *s) : mRawPtr(s) { }
> 63
> 64    template<class Dest>
> 65    operator Dest*() {
> 66      if (!mRawPtr)
> 67        return nullptr;
> 68
> 69      return reinterpret_cast<Dest*>(mRawPtr->QueryFrame(Dest::kFrameIID));
> 70    }
> 71
> 72  private:
> 73    nsQueryFrame *mRawPtr;
> 74  };
> 75

ViewportFrame::kFrameIID is just an alias for nsContainerFrame::kFrameIID
so the QueryFrame() checks if this is a nsContainerFrame and then casts the
result to ViewportFrame.

Is there a way to make the example fail to compile using some C++ trickery?

If not, then I suggest we make kFrameIID mandatory for all frame classes.

Comment 1

[Jeff Walden [:Waldo]]

Instead of ViewportFrame::kFrameIID, you could have

  template<class C> struct FrameId;
  template<> struct FrameId<nsContainerFrame> { static const nsQueryFrame::FrameIID id = ...; };

and so on, generated with slightly different macro hackery.  Any attempt to get FrameId<ViewportFrame> would hit the undefined case and die.  I don't know enough about frame class ids to know if this would also require having every class have a frame id or not.

Comment 2

[Jeff Walden [:Waldo]]

Erm.  I meant to CC jcranmer because I wondered if MOZ_MUST_OVERRIDE might have any vague applicability to static consts in classes.  That might also be a fix too.

Comment 3

[Mats Palmgren (inactive)]

NS_DECL_QUERYFRAME_TARGET is in class scope, so clang complains over having
a template specialization there.  What we currently have is this:

class nsContainerFrame : ... {
public:
  NS_DECL_QUERYFRAME_TARGET(nsContainerFrame)
...
};

class ViewportFrame: public nsContainerFrame {
 // no NS_DECL_QUERYFRAME_TARGET here!
};

For the above we want:
  nsContainerFrame* x = do_QueryFrame(a); // ok
  ViewportFrame* y = do_QueryFrame(a); // should fail to compile


All classes have a unique constant in an enum, which is the value
NS_DECL_QUERYFRAME_TARGET refers to as nsQueryFrame::classname##_id

It's not a requirement that do_QueryFrame should fail for ViewportFrame
per se, so adding NS_DECL_QUERYFRAME_TARGET everywhere is a solution.

Just wondering if there exist a solution to add them as needed,
without the foot gun :-)

Comment 4

[Mats Palmgren (inactive)]

A simple assignment check seems to do what I want, like so:

#define NS_DECL_QUERYFRAME_TARGET(classname)                    \
  static const nsQueryFrame::FrameIID kFrameIID = nsQueryFrame::classname##_id; \
  static classname* do_QueryFrame_helper() {return nullptr;}


class do_QueryFrame
{
public:
  do_QueryFrame(nsQueryFrame *s) : mRawPtr(s) { }

  template<class Dest>
  operator Dest*() {
    Dest* dummy;
    MOZ_STATIC_ASSERT(!(dummy = Dest::do_QueryFrame_helper()), "");
    if (!mRawPtr)
      return nullptr;

    return reinterpret_cast<Dest*>(mRawPtr->QueryFrame(Dest::kFrameIID));
  }

private:
  nsQueryFrame *mRawPtr;
};


... and with that I find two existing bugs in the tree.
One is in a MOZ_ASSERT which seems safe, the other I'm not sure...
so hiding the bug for now.

Comment 5

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/annotate/8693d1d4c86d/layout/generic/nsSimplePageSequence.cpp#l496

nsHTMLCanvasFrame doesn't have a NS_DECL_QUERYFRAME_TARGET so it
inherits nsContainerFrame::kFrameIID.  So if 'child' is a nsContainerFrame
of some sort then 'canvasFrame' will be non-null.

Comment 6

[Mats Palmgren (inactive)]

Attachment: fix

We can use the 'return nullptr' for the assignment check.
The clang error message for a missing NS_DECL_QUERYFRAME_TARGET is
quite clear I think:

layout/generic/nsQueryFrame.h:68:14: error: cannot initialize return object of type 'nsHTMLCanvasFrame *' with an rvalue of type 'typename nsHTMLCanvasFrame::Has_NS_DECL_QUERYFRAME_TARGET' (aka 'nsContainerFrame *')
      return static_cast<typename Dest::Has_NS_DECL_QUERYFRAME_TARGET>(nullptr);
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
layout/generic/nsSimplePageSequence.cpp:496:40: note: in instantiation of function template specialization 'do_QueryFrame::operator class nsHTMLCanvasFrame *<nsHTMLCanvasFrame>' requested here
      nsHTMLCanvasFrame* canvasFrame = do_QueryFrame(child);
                                       ^

=========

https://tbpl.mozilla.org/?tree=Try&rev=941c357ca3f3

Comment 7

[Joshua Cranmer [:jcranmer]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Erm.  I meant to CC jcranmer because I wondered if MOZ_MUST_OVERRIDE might
> have any vague applicability to static consts in classes.  That might also
> be a fix too.

MOZ_MUST_OVERRIDE is defined for methods, and is tested to work for non-virtual methods. Given its design, it may even work for static methods (I hadn't considered that possibility before)... As it stands, it doesn't work for fields, though.

Comment 8

[Zack Weinberg (:zwol)]

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

Review of attachment 731540 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsQueryFrame.h
@@ +64,5 @@
>  
>    template<class Dest>
>    operator Dest*() {
>      if (!mRawPtr)
> +      return static_cast<typename Dest::Has_NS_DECL_QUERYFRAME_TARGET>(nullptr);

This is sufficiently magic that I think a comment of some sort is in order.  Perhaps just 

 // Ensure that Dest declared itself as a queryframe target.

Comment 9

[Mats Palmgren (inactive)]

Attachment: fix, v2

With the comment Zack suggested.

Comment 10

[Mats Palmgren (inactive)]

GetPrintCanvasElementsInFrame is recursive and the only other caller is
nsSimplePageSequenceFrame::PrePrintNextPage
http://dxr.mozilla.org/mozilla-central/layout/generic/nsSimplePageSequence.cpp#l605
calling it 'mCurrentPageFrame', so it seems there's no risk it can be
affected by unprivileged content.  'canvasFrame->GetContent()' is the only
use of 'canvasFrame' anyway and that's safe regardless of frame type.

Comment 11

[Daniel Holbert [:dholbert]]

That all appears to be true. (and the nsHTMLFramesetBlankFrame do_QueryFrame call isn't exploitable in release builds, because it's not compiled into those builds -- it's part of a debug-only assertion.)

So I don't think this is exploitable (and hence, this patch isn't going to zero-day our branch releases when it's landed).

Comment 12

[Mats Palmgren (inactive)]

Backed it out because of:
Assertion failure: blank = do_QueryFrame(child) (unexpected child frame type), at layout/generic/nsFrameSetFrame.cpp:1052

Odd because it appears to be correct:

1052  MOZ_ASSERT(blank = do_QueryFrame(child), "unexpected child frame type");
(gdb) p *child
$1 = (nsHTMLFramesetBlankFrame) {
  <nsLeafFrame> = {
    <nsFrame> = {

Comment 13

[Joshua Cranmer [:jcranmer]]

(In reply to Mats Palmgren [:mats] from comment #12)
> Backed it out because of:
> Assertion failure: blank = do_QueryFrame(child) (unexpected child frame
> type), at layout/generic/nsFrameSetFrame.cpp:1052
> 
> Odd because it appears to be correct:
> 
> 1052  MOZ_ASSERT(blank = do_QueryFrame(child), "unexpected child frame
> type");

I don't see nsHTMLFramesetBlankFrame declaring NS_DECL_QUERYFRAME_TARGET?

Comment 14

[Daniel Holbert [:dholbert]]

You're correct, it doesn't (and mats' patch fixes that).

Comment 15

[Jeff Walden [:Waldo]]

(In reply to Mats Palmgren [:mats] from comment #12)
> Assertion failure: blank = do_QueryFrame(child) (unexpected child frame
> type), at layout/generic/nsFrameSetFrame.cpp:1052
> 
> 1052  MOZ_ASSERT(blank = do_QueryFrame(child), "unexpected child frame
> type");
> (gdb) p *child
> $1 = (nsHTMLFramesetBlankFrame) {
>   <nsLeafFrame> = {
>     <nsFrame> = {

Uh, it's surely not a good idea to have |=| there, even if those are the semantics you want!

Comment 16

[Jeff Walden [:Waldo]]

Hmm, so an assignment there *does* work.  It just reads horribly wrong to see a presumptively-side-effecting assignment in an assertion condition.  :-\  Guh.

Comment 17

[Mats Palmgren (inactive)]

I blame DebugOnly<> that evaluates also in non-debug builds... ;-P

Comment 18

[Mats Palmgren (inactive)]

It seems I misunderstood what NS_DECL_QUERYFRAME_TARGET is for.
Here's an example to illustrate, nsIStatefulFrame:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsIStatefulFrame.h
which is then added by the concrete class:
http://hg.mozilla.org/mozilla-central/annotate/0b7c27024048/layout/generic/nsGfxScrollFrame.cpp#l868

So it's just a way to add additional targets for a concrete class that MUST
implement QueryFrame.  NS_DECL_QUERYFRAME_TARGET doesn't do anything by itself.

Comment 19

[Mats Palmgren (inactive)]

Attachment: fix, v3

So we also need NS_DECL_QUERYFRAME + implementation.

I also added an assertion in NS_QUERYFRAME_TAIL_INHERITANCE_ROOT
which is only implemented in the top QueryFrame (in nsFrame).
So if the target ID did not match any of the supported QF IDs,
then it must not match the real ID of the instance either.
If it does then you added NS_DECL_QUERYFRAME_TARGET without
actually implementing it.  This asserts for the v2 patch.

This patch pass reftest/crashtests locally (Linux64).
Let's see what Try says:
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=952c8f1a3eeb

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 732103 [details] [diff] [review]
fix, v3

> #define NS_QUERYFRAME_TAIL_INHERITANCE_ROOT                     \
>   default: break;                                               \
>   }                                                             \
>-  return nullptr;                                                \
>+  MOZ_ASSERT(id != GetFrameId(),                                \
>+    "NS_DECL_QUERYFRAME_TARGET without NS_DECL_QUERYFRAME ?");  \
>+  return nullptr;                                               \

The assertion-message could stand to be clarified a bit.

Maybe replace the assertion with:
  MOZ_ASSERT(id != GetFrameId(),                                 \
    "A frame failed to QueryFrame to its *own type*. It may be " \
    "missing NS_DECL_QUERYFRAME, or a NS_QUERYFRAME_ENTRY() "    \
    "line with its own type name");                              \

(or, add a comment to that effect)

r=me with that.

Also, can we un-hide this bug, given that comment 10 appears to address your security concerns at the end of comment 4?

Comment 21

[Zack Weinberg (:zwol)]

I want to ask you to change this comment again...

    if (!mRawPtr) {
      // Ensure that Dest declared itself as a queryframe target.
      return static_cast<typename Dest::Has_NS_DECL_QUERYFRAME_TARGET>(nullptr);
    }

Looking at the larger context, it reads as if we're only doing the "Ensure ..." when !mRawPtr.  This would be clearer:

  // This expression doubles as a compile-time assertion that
  // Dest declared itself as a queryframe target.

Tweak wording if you like; the important thing is "compile time".

Comment 22

[Mats Palmgren (inactive)]

I will improve both comments as you suggested, thanks.

(In reply to Daniel Holbert [:dholbert] from comment #20)
> Also, can we un-hide this bug, given that comment 10 appears to address your
> security concerns at the end of comment 4?

Maybe.  I'm still a little worried over how error prone this code is
so there could be additional problems we haven't found yet...

So I ran a few scripts over the tree to gather some statistics:

We have 60 unique NS_QUERYFRAME_HEAD
We have 54 unique NS_DECL_QUERYFRAME_TARGET
We have 55 unique NS_QUERYFRAME_ENTRY  (huh? one more??)
We have 0 NS_QUERYFRAME_ENTRY_CONDITIONAL

The ENTRY that does not have a TARGET is:
nsMathMLFrame

http://hg.mozilla.org/mozilla-central/annotate/aae004a3c5d9/layout/mathml/nsMathMLContainerFrame.cpp#l41

Is that ENTRY supposed to be nsIMathMLFrame ?

There are ~17 or so do_QueryFrame calls with nsIMathMLFrame as target.
That doesn't seem right.  Are all those safe?

Comment 23

[Mats Palmgren (inactive)]

Attachment: experiment 2

So I did this experiment and looked at the actual frames when
I got an assertion in gdb.  Here's a few:

(gdb) p *this
$1 = (nsMathMLmrowFrame) {
  <nsMathMLContainerFrame> = {
    <nsContainerFrame> = {
      <nsSplittableFrame> = {
        <nsFrame> = {
          <nsBox> = {
            <nsIFrame> = {
              <nsQueryFrame> = {
                _vptr$nsQueryFrame = 0x7ffff635f710
              }, 

(gdb) p *this
$2 = (nsMathMLmstyleFrame) {
  <nsMathMLContainerFrame> = {
    <nsContainerFrame> = {
      <nsSplittableFrame> = {
        <nsFrame> = {
          <nsBox> = {
            <nsIFrame> = {
              <nsQueryFrame> = {
                _vptr$nsQueryFrame = 0x7ffff6364d80
              }, 

(gdb) p *this
$3 = (nsMathMLTokenFrame) {
  <nsMathMLContainerFrame> = {
    <nsContainerFrame> = {
      <nsSplittableFrame> = {
        <nsFrame> = {
          <nsBox> = {
            <nsIFrame> = {
              <nsQueryFrame> = {
                _vptr$nsQueryFrame = 0x7ffff635ec10
              }, 


I get the impression most MathML frame classes don't QueryFrame
to nsIMathMLFrame.  This seems wrong.

Comment 24

[Mats Palmgren (inactive)]

Attachment: part 2, Require that NS_QUERYFRAME_ENTRY[_CONDITIONAL] IDs are declared in the class they are testing

This is the same compile-time check that we did in do_QueryFrame.
This makes the "NS_QUERYFRAME_ENTRY(nsMathMLFrame)" fail to compile
since the nsMathMLFrame class does not have a TARGET.

Comment 25

[Mats Palmgren (inactive)]

Attachment: part 3, Fix typo that caused most MathML frames to not QueryFrame to nsIMathMLFrame

So we should either remove this entry, since it now won't compile,
or change it to nsIMathMLFrame to make nsMathMLContainerFrame and
derived classes QueryFrame to nsIMathMLFrame.  This seems reasonable
since nsMathMLContainerFrame inherits nsMathMLFrame which inherits
nsIMathMLFrame.  (I'll submit a Try run in a bit...)

Comment 26

[Mats Palmgren (inactive)]

For the record, here are some additional statistics:

TARGETs with no corresponding HEAD:

nsISVGSVGFrame
nsISVGGlyphFragmentNode
nsISVGChildFrame
nsIAnonymousContentCreator
nsIPageSequenceFrame
nsIStatefulFrame
nsIObjectFrame
nsIScrollableFrame
nsIFrame
nsIMathMLFrame
nsIPercentHeightObserver
nsIFormControlFrame
nsITextControlFrame
nsISelectControlFrame
nsIListControlFrame
nsIComboboxControlFrame
nsIScrollbarMediator
nsIRootBox
nsITableCellLayout

(all abstract classes so that seems reasonable)

HEADs with no corresponding TARGET:

nsSVGOuterSVGFrame
nsSVGForeignObjectFrame
nsSVGUseFrame
nsSVGTSpanFrame
nsSVGGlyphFrame
nsFrame
nsSimplePageSequenceFrame
nsVideoFrame
nsHTMLScrollFrame
nsXULScrollFrame
nsMathMLContainerFrame
nsMathMLmtableOuterFrame
nsMathMLmtdInnerFrame
nsListControlFrame
nsComboboxControlFrame
nsFormControlFrame
nsHTMLButtonControlFrame
nsFileControlFrame
nsImageControlFrame
nsGfxButtonControlFrame
nsTextControlFrame
nsRootBoxFrame
nsDocElementBoxFrame
nsTableFrame

We should probably audit these for mistakes.
For example, nsTableFrame is empty which seems harmless but useless:
http://hg.mozilla.org/mozilla-central/annotate/aae004a3c5d9/layout/tables/nsTableFrame.cpp#l163

Comment 27

[Mats Palmgren (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=b7c186d13b6b
https://tbpl.mozilla.org/?tree=Try&rev=463a99541ae9

Comment 28

[Mats Palmgren (inactive)]

# grep -r 'public nsQueryFrame' .
./layout/svg/nsISVGGlyphFragmentNode.h:class nsISVGGlyphFragmentNode : public nsQueryFrame
./layout/svg/nsISVGChildFrame.h:class nsISVGChildFrame : public nsQueryFrame
./layout/generic/nsIPageSequenceFrame.h:class nsIPageSequenceFrame : public nsQueryFrame
./layout/generic/nsIObjectFrame.h:class nsIObjectFrame : public nsQueryFrame
./layout/generic/nsIScrollableFrame.h:class nsIScrollableFrame : public nsQueryFrame {
./layout/generic/nsIFrame.h:class nsIFrame : public nsQueryFrame
./layout/forms/nsIFormControlFrame.h:class nsIFormControlFrame : public nsQueryFrame
./layout/forms/nsISelectControlFrame.h:class nsISelectControlFrame : public nsQueryFrame
./layout/forms/nsIListControlFrame.h:class nsIListControlFrame : public nsQueryFrame
./layout/forms/nsIComboboxControlFrame.h:class nsIComboboxControlFrame : public nsQueryFrame

which means all frame classes that inherits those will have an
additional vtable pointer for nsQueryFrame per interface.  sigh.

class nsListControlFrame : public nsHTMLScrollFrame,
                           public nsIFormControlFrame, 
                           public nsIListControlFrame,
                           public nsISelectControlFrame

Poor nsListControlFrame, it will have 5!

$1 = (nsHTMLScrollFrame) {
  <nsHTMLScrollFrame> = {
    <nsContainerFrame> = {
      <nsSplittableFrame> = {
        <nsFrame> = {
          <nsBox> = {
            <nsIFrame> = {
              <nsQueryFrame> = {
                _vptr$nsQueryFrame = 0x7ffff6340ad0
              }, 
...
    <nsIScrollableFrame> = {
      <nsQueryFrame> = {
        _vptr$nsQueryFrame = 0x7ffff6341088
      }, <No data fields>}, 
...
  <nsIFormControlFrame> = {
    <nsQueryFrame> = {
      _vptr$nsQueryFrame = 0x7ffff65461c0
    }, <No data fields>}, 
  <nsIListControlFrame> = {
    <nsQueryFrame> = {
      _vptr$nsQueryFrame = 0x7ffff65462b0
    }, <No data fields>}, 
  <nsISelectControlFrame> = {
    <nsQueryFrame> = {
      _vptr$nsQueryFrame = 0x0
    }, <No data fields>}, 
...

Comment 29

[Mats Palmgren (inactive)]

Attachment: part 2, Require that NS_QUERYFRAME_ENTRY[_CONDITIONAL] IDs are declared in the class they are testing

Old version didn't compile on Windows due to the 'typename', let's try without
https://tbpl.mozilla.org/?tree=Try&rev=a63f7377ac75

Comment 30

[Jeff Walden [:Waldo]]

If you're doing a compile-time assertion, you might as well make it a static assertion, so that when compiling as C++11 (which Gecko does, I believe) you'll get an even better error message.  Like so, say:

  MOZ_STATIC_ASSERT((mozilla::IsSame<Dest, typename Dest::Has_NS_DECL_QUERYFRAME_TARGET>::value),
                    "Dest must declare itself as a queryframe target");

This'll turn into a static_assert(..., "Dest must...") in those builds and report a compile error with exactly that string as the error output.  Hard to beat that!

IsSame (mozilla/TypeTraits.h) looks like what you wanted for the original patch.  I think you might want IsBaseOf in some of the newer patches, but there's enough churn/traffic here I'm no longer certain what's actually wanted here.  :-)

Comment 31

[Daniel Holbert [:dholbert]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #30)
> IsSame (mozilla/TypeTraits.h) looks like what you wanted for the original
> patch.  I think you might want IsBaseOf in some of the newer patches

I'm actually pretty sure we want to use IsSame() in part 2, as well. (Not IsBaseOf.)  There *is* converting to a base class going on, but the assertion is about "Does this particular base class 'class' explicitly declare itself as a QueryFrame target?" which involves checking whether 'class' and 'class:::Has_NS_DECL_QUERYFRAME_TARGET' are the same type.

So -- mats, the new patches look good, but could you look into using MOZ_STATIC_ASSERT / IsSame as Waldo suggests in comment 30, for parts 1 and 2? If that works, I'd prefer it, because it's much more human-readable to actually use assertions to assert stuff, and it also sounds like it'll produce saner output when it fails.

Comment 32

[Benjamin Smedberg]

Couple notes here:

1) http://mxr.mozilla.org/mozilla-central/source/layout/generic/frame-verify.js was designed to do static verification of this, except of course that we never got dehydra-based static analysis working. Oh well!

2) This is very similar to bug 514280. I think that we could use template specialization instead of an accessor method to do this, e.g.

template<class Target> struct FrameTypeInfo;

// specialize for each class
template<>
struct FrameTypeInfo<nsIMathMLFrame>
{
  static const nsQueryFrame::FrameIID kIID = nsQueryFrame::nsIMathMLFrame;
};

I'm not sure whether you could even do this all in nsQueryFrame.h with forward-declared classes... I'm going to try it.

Comment 33

[Mats Palmgren (inactive)]

Attachment: fix, v4

Changed the typecasts to MOZ_STATIC_ASSERT of mozilla::IsSame, as requested.
And yes, the error messages looks nicer.

I folded part 2 into the first patch so you can see all nsQueryFrame.h
changes in the same patch.  (only nsQueryFrame.h needs a new review)

Comment 34

[Daniel Holbert [:dholbert]]

Comment on attachment 732516 [details] [diff] [review]
fix, v4

Looks good!

When landing, you should either:
 a) fold "part 3" (the nsIMathMLFrame tweak) into the main patch
...OR...
 b) land "part 3" first (*before* the main patch)

Otherwise, you'll be introducing an intermediate changeset that doesn't build successfully (due to the failing MOZ_STATIC_ASSERT), which is undesirable from the perspective of anyone who uses 'hg bisect' to track down some unrelated issue and happens to hit your intermediate changeset and can't build.

I'd lean towards just folding it all together, since you've already got the other random-frameclass-fixups as being part of the main fix, and the nsIMathMLFrame tweak isn't fundamentally any different from those.

Comment 35

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c94dce47533

Comment 36

[[e-mail]]

Attachment: Alternative approach as per comment #32

(In reply to Benjamin Smedberg from comment #32)
> 2) This is very similar to bug 514280. I think that we could use template
> specialization instead of an accessor method to do this
Just for completeness, this is how the patch would have looked.

Obviously this approach found the same compile errors.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

(In reply to Mats Palmgren [:mats] from comment #35)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/0c94dce47533

https://hg.mozilla.org/mozilla-central/rev/0c94dce47533