1231132 - [Static Analysis][Uninitialized scalar field] SheetLoadData::SheetLoadData from Loader.cpp

Status: RESOLVED WORKSFORME

(Core :: CSS Parsing and Computation, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity added that member variable mStatus is not initialized in constructors.

Comment 1

[Andi [:andi]]

Attachment: Bug 1231132.diff

Hello David,

Can you please take a look other this patch. it's not big deal, just added a member variable to the constructors.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8696691 [details] [diff] [review]
Bug 1231132.diff

This is probably ok, but you should update the comment which explains when this is initialized/used to talk about setting, not initializing, right?

Comment 3

[Andi [:andi]]

Hello Boris,

Thank you for letting me know. The variable - mStatus should be set in the 3 constructors that have parameters. 
Other place where mStatus is set is in function SheetLoadData::ScheduleLoadEventIfNeeded:

>>  if (!mOwningElement) {
>>    return;
>>  }

>>  mStatus = aStatus;

ScheduleLoadEventIfNeeded gets called from Loader::DoSheetComplete:

>>    if (!data->mSheetAlreadyComplete) {
>>      // If mSheetAlreadyComplete, then the sheet could well be modified between
>>      // when we posted the async call to SheetComplete and now, since the sheet
>>      // was page-accessible during that whole time.
>>      MOZ_ASSERT(!data->mSheet->IsModified(),
>>                 "should not get marked modified during parsing");
>>      data->mSheet->SetComplete();
>>      data->ScheduleLoadEventIfNeeded(aStatus);
>>    }

On the other hand Loader::DoSheetComplete gets called on 2 routes:

1. from itself recursively:
>>    if (data->mParentData &&
>>        --(data->mParentData->mPendingChildren) == 0 &&
>>        !mParsingDatas.Contains(data->mParentData)) {
>>      DoSheetComplete(data->mParentData, aStatus, aDatasToNotify);
>>    }

2. from path: SheetLoadData::Run(..)->Loader::HandleLoadEvent(..)->Loader::SheetComplete(..)->Loader::DoSheetComplete(..)

I cannot tell you for sure that having mStatus not set in those constructors can cause a bug, the fix is more to silence Coverity. If you need more info please let me know and i'll do my best to privide it.

THX

Comment 4

[Boris Zbarsky [:bzbarsky]]

There is no actual bug here: the only code that _reads_ mStatus runs only if ScheduleLoadEventIfNeeded has been called.  So the patch is not really needed, but doesn't hurt anything either.

My point was that the patch should update the comment that explains all this and describes the current setup (which you're changing).  That comment is right above the declaration of mStatus.

Comment 5

[Andi [:andi]]

Thx, now i understand, i didn't got that you were referring to the actual comment from the cpp.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8696691 [details] [diff] [review]
Bug 1231132.diff

Could you update the patch as requested?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That said, I also have mixed feelings about taking patches of this sort, since it means we lose the potential to learn about problems from valgrind warnings.  I guess I'm ok with them, though.

Comment 8

[Andi [:andi]]

Attachment: MozReview Request: Bug 1231132 - setting mStatus in constructors to NS_OK in order to silence a Coverity checker. It's not a bug since this gets set by ScheduleLoadEventIfNeeded, and is only used after that has been called. r?dbaron

Review commit: https://reviewboard.mozilla.org/r/34343/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34343/

Comment 9

[Andi [:andi]]

David if toy want there is always the possibility to wave it from Coverity and mark it as ignore.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8717824 [details]
MozReview Request: Bug 1231132 - setting mStatus in constructors to NS_OK in order to silence a Coverity checker. It's not a bug since this gets set by ScheduleLoadEventIfNeeded, and is only used after that has been called. r?dbaron

https://reviewboard.mozilla.org/r/34343/#review32349

Could you add a call to MOZ_MAKE_MEM_UNDEFINED() in all 3 constructors to declare that the memory occupied by mStatus is still considered undefined?  That would address the valgrind issue and also avoid the need to update the comment as Boris requested in comment 2 and comment 4 (and which I'm a little surprised that this patch hasn't updated, given those comments