Closed Bug 931912 Opened 6 years ago Closed 6 years ago
Suppress an exact rooting hazard false positive in ns
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.
Comment on attachment 823446 [details] [diff] [review] suppress_nsWindowSH_Finalize-v0.diff 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: https://hg.mozilla.org/integration/mozilla-inbound/rev/83c4706d9fe3 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.
https://tbpl.mozilla.org/?tree=Try&rev=8e5e7fc94a7e 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. https://tbpl.mozilla.org/?tree=Try&rev=21463f4415f2
That seemed to work better. No new code in mfbt, so no new review needed: https://hg.mozilla.org/integration/mozilla-inbound/rev/0b5ffddb8bad
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.