Closed Bug 1374236 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1412671)

Attachments

(1 file)

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

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
Attachment #8879071 - Flags: review?(mats) → review+
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a064e2281012
Refactor MaybeSetupTransactionIdAllocator to avoid passing nsView*. r=mats
https://hg.mozilla.org/mozilla-central/rev/a064e2281012
Status: NEW → RESOLVED
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.