Closed Bug 931912 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: terrence, Assigned: terrence)

References

Details

(Whiteboard: [qa-])

Attachments

(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]
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
https://hg.mozilla.org/mozilla-central/rev/0b5ffddb8bad
Status: ASSIGNED → RESOLVED
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.