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).

Brad, could you take this?

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

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 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.)

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

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].

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 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 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+.