1421420 - GetGridFrameWithComputedInfo probably needs to be more careful about data being destroyed across its shell->FlushPendingNotifications call

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Right now, nsGridContainerFrame::GetGridFrameWithComputedInfo has the following code:

==========
nsGridContainerFrame*
nsGridContainerFrame::GetGridFrameWithComputedInfo(nsIFrame* aFrame)
{
  [...]
      shell->FlushPendingNotifications(FlushType::Layout);

      // Since the reflow may have side effects, get the grid frame again.
      gridFrame = GetGridContainerFrame(aFrame);

https://dxr.mozilla.org/mozilla-central/rev/cad9c9573579698c223b4b6cb53ca723cd930ad2/layout/generic/nsGridContainerFrame.cpp#6931-6934
==========

This pattern is problematic & potentially dangerous.  FlushPendingNotifications causes events to be flushed, which means it can cause script to synchronously run and e.g. tear down the document or a piece of the frame tree.  So, by the time we get to the  "GetGridContainerFrame(aFrame)" call, aFrame may be pointing to bogus/freed memory -- so GetGridContainerFrame's usages of aFrame are potentially-unsafe in that scenario.

Fortunately in this case, frame poisoning should make any resulting crash a "safe"/non-exploitable crash. But, still worth avoiding & worrying about.

This is probably a case where we want to use AutoWeakFrame:
https://dxr.mozilla.org/mozilla-central/rev/cad9c9573579698c223b4b6cb53ca723cd930ad2/layout/generic/nsIFrame.h#4571

Or possibly a persistent nsCOMPtr<nsIContent*> which we'd use to re-discover the frame after the layout flush, as it looks like Brad's doing in the flexbox equivalent function over in bug 1409083 (modulo my nsCOMPtr review-feedback over there).

Comment 1

[Daniel Holbert [:dholbert]]

Brad, could you take this?

Comment 2

[Brad Werth [:bradwerth] (Out until Nov 14)]

Thanks for finding this. I was thinking of this same issue after seeing your comments on the similar flex computed info.

Comment 3

[Daniel Holbert [:dholbert]]

Sure!  Yeah, I wanted to be sure my comments on the flex bug wouldn't cause a zero-day for the similar grid code. :) Was happy to see that this version *looks* like a safe frame-poisoning crash, at worst.

Comment 4

[Brad Werth [:bradwerth] (Out until Nov 14)]

Attachment: Bug 1421420 Part 1: Hold onto a weak reference to the grid container frame across reflows.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8932643 [details] [diff] [review]
Bug 1421420 Part 1: Hold onto a weak reference to the grid container frame across reflows.

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

Thanks! r=me, just one commit message nit:
> Hold onto a weak reference to the grid container frame across reflows

s/reflows/grid-devtools reflow flush/

(or something like that. The current text sounds like it's doing this for all or most reflows in general.)

Comment 6

[Brad Werth [:bradwerth] (Out until Nov 14)]

Attachment: Bug 1421420 Part 1: Hold onto a weak reference to the grid container frame across reflow flushes triggered by devtools.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

This needs a sec rating (and possibly sec-approval depending on said rating) before it can land.

Comment 8

[Mats Palmgren (inactive)]

This is a good fix, but I think the issue is mostly theoretical since
this method is only called from DoGridTemplateColumns/Rows and those
properties are marked as needing a layout flush:
https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/layout/style/nsCSSPropList.h#2279,2292
so by the time this method is called all layout data is already
up-to-date.  So this (2nd) flush should be a NOP except for forcing
another reflow on the grid frame [to collect the requested data].

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9982f241548d0a1745e7625a4554cb76362151d1

Please request Beta approval on this when you get a chance. It grafts cleanly as-landed.

Comment 10

[Brad Werth [:bradwerth] (Out until Nov 14)]

Comment on attachment 8932660 [details] [diff] [review]
Bug 1421420 Part 1: Hold onto a weak reference to the grid container frame across reflow flushes triggered by devtools.

Approval Request Comment
[Feature/Bug causing the regression]: 1241932
[User impact if declined]: Future patches may introduce security issues if they add more triggers for this code. Current invocation patterns appear safe.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only triggered by use of specific features in devtools. 
[String changes made/needed]: None

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9982f241548d

Comment 12

[Gerry Chang [:gchang]]

Comment on attachment 8932660 [details] [diff] [review]
Bug 1421420 Part 1: Hold onto a weak reference to the grid container frame across reflow flushes triggered by devtools.

Fix a security issue. Beta58+.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7649c04f02c3