Closed Bug 931912 Opened 6 years ago Closed 6 years ago

Suppress an exact rooting hazard false positive in nsWindowSH::Finalize


(Core :: DOM: Core & HTML, defect)

Not set





(Reporter: terrence, Assigned: terrence)



(Whiteboard: [qa-])


(1 file, 1 obsolete file)

Quoting from the patch:
+  // Since this call is virtual, the exact rooting hazard static analysis is
+  // not able to determine that it happens during finalization and should be
+  // ignored. Moreover, the analysis cannot discover and validate the
+  // potential targets of the virtual call to OnFinalize below because of the
+  // indirection through nsCOMMPtr. Thus, we annotate the analysis here so
+  // that it does not report OnFinalize as GCing with |obj| on stack.

At least, this is how I interpret what is going on here. Steve, does the above sound reasonable to you?
Attachment #823446 - Flags: review?(sphink)
s/nsCOMMPtr/nsCOMPtr/ and seems fine.
Duplicate of this bug: 908908
Comment on attachment 823446 [details] [diff] [review]

Review of attachment 823446 [details] [diff] [review]:

Sorry for the delay. I'm still thinking the analysis might be able to figure this out from its callers (as in, it should know that finalization cannot GC.) But I haven't looked to see if there are nasty problems with that approach. So this is good.
Attachment #823446 - Flags: review?(sphink) → review+
And backed out for opt bustage by:

Normally, the only use is |this|. Even when the class is empty, there are implicit constructors and destructors that "run", but since there is also no data in the class |this| is never read or written. It turns out the opt build I did before pushing did have this warning, but for some reason my local build is not respecting warn-as-error. Bummer.

Added MOZ_UNUSED. Will flag for review of the new bits if Try is green.
Attachment #823446 - Attachment is obsolete: true
And of course windows still complains because that technique can't work there. Lets try adding an empty constructor.
That seemed to work better. No new code in mfbt, so no new review needed:
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.