The Static Analysis tool Coverity detected that an explicit null pointer dereference can occur in this context on pointer |view|: (1) >> nsView *view = nullptr; >> if (aFlags & PAINT_USE_WIDGET_LAYERS) { >> layerManager = aBuilder->GetWidgetLayerManager(&view); >> if (layerManager) { >> doBeginTransaction = !(aFlags & PAINT_EXISTING_TRANSACTION); >> widgetTransaction = true; >> } >> } (2) >> MaybeSetupTransactionIdAllocator(layerManager, view); I don't think that this can be fixed by adding (2) in the if statement: >> bool shouldInvalidate = layerManager->NeedsWidgetInvalidation(); >> if (view) { >> if (props) { >> if (!invalid.IsEmpty()) { Since MaybeSetupTransactionIdAllocator should be before: >> layerManager->EndTransaction(FrameLayerBuilder::DrawPaintedLayer, >> aBuilder, flags);

Comment on attachment 8879071 [details] Bug 1374236 - Refactor MaybeSetupTransactionIdAllocator to avoid passing nsView*. https://reviewboard.mozilla.org/r/150396/#review155026 I'm pretty sure this is a false positive and I'm reluctant to add runtime cost just to satisfy Coverity. It seems MaybeSetupTransactionIdAllocator doesn't actually need a view though: http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/layout/base/nsLayoutUtils.cpp#8611 Could we change that param to a nsPresContext* instead, and then pass |presContext| here, and the same in PresShell::Paint? That should be the same pres context as we get via the view. Also, while we're here, could you add a temporary variable for the GetBackendType() result? It's a virtual method and it seems unnecessary to call it twice. Something like: auto backendType = aManager->GetBackendType(); if (backendType == ...

One more thing, regarding the coding style, i've applied on the patch clang-format with our coding style as a template.

Comment on attachment 8879071 [details] Bug 1374236 - Refactor MaybeSetupTransactionIdAllocator to avoid passing nsView*. https://reviewboard.mozilla.org/r/150396/#review155180 ::: layout/base/nsLayoutUtils.h:3080 (Diff revision 2) > nsPresContext *mPresContext; > bool mOldValue; > }; > > - void MaybeSetupTransactionIdAllocator(layers::LayerManager* aManager, nsView* aView); > + void MaybeSetupTransactionIdAllocator(layers::LayerManager* aManager, > + nsPresContext* presContext); The parameter should be named aPresContext to follow our naming conventions. Looks great otherwise. r=mats with that fixed

Pushed by bpostelnicu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a064e2281012 Refactor MaybeSetupTransactionIdAllocator to avoid passing nsView*. r=mats