Closed Bug 1374236 Opened 3 years ago Closed 3 years ago

[Static Analysis][Explicit null dereferenced] In function nsDisplayList::PaintRoot


(Core :: Layout, defect)

Not set



Tracking Status
firefox56 --- fixed


(Reporter: andi, Assigned: andi)


(Blocks 1 open bug)


(Keywords: coverity, Whiteboard: CID 1412671)


(1 file)

The Static Analysis tool Coverity detected that an explicit null pointer dereference can occur in this context on pointer |view|:


>>  nsView *view = nullptr;
>>  if (aFlags & PAINT_USE_WIDGET_LAYERS) {
>>    layerManager = aBuilder->GetWidgetLayerManager(&view);
>>    if (layerManager) {
>>      doBeginTransaction = !(aFlags & PAINT_EXISTING_TRANSACTION);
>>      widgetTransaction = true;
>>    }
>>  }

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

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:
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 == ...
Attachment #8879071 - Flags: review?(mats) → review-
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*.

::: 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
Attachment #8879071 - Flags: review?(mats) → review+
Pushed by
Refactor MaybeSetupTransactionIdAllocator to avoid passing nsView*. r=mats
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.