853607 - Mousing across a <video controls> element triggers tons of "WARNING: NS_ENSURE_TRUE(wrapper) failed: file dom/base/nsJSUtils.cpp, line 84"

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Load data:text/html,<video controls> in a debug build
 2. Mouse over the video

ACTUAL RESULTS:
We spam tons of instances of this warning to the terminal:
> WARNING: NS_ENSURE_TRUE(wrapper) failed: file /mozilla/dom/base/nsJSUtils.cpp, line 84

We spam an instance for every mousemove event on top of the <video>, it looks like.

Comment 1

[Daniel Holbert [:dholbert]]

The code in question is:
====
50 nsIScriptGlobalObject *
51 nsJSUtils::GetStaticScriptGlobal(JSObject* aObj)
52 {
[...]
75   // We might either have a window directly (e.g. if the global is a
76   // sandbox whose script object principal pointer is a window), or an
77   // XPCWrappedNative for a window.  We could also have other
78   // sandbox-related script object principals, but we can't do much
79   // about those short of trying to walk the proto chain of |glob|
80   // looking for a window or something.
81   nsCOMPtr<nsIScriptGlobalObject> sgo(do_QueryInterface(supports));
82   if (!sgo) {
83     nsCOMPtr<nsIXPConnectWrappedNative> wrapper(do_QueryInterface(supports));
84     NS_ENSURE_TRUE(wrapper, nullptr);
85     sgo = do_QueryWrappedNative(wrapper);
86   }
====
https://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSUtils.cpp#75

and we're failing the NS_ENSURE_TRUE, because our 'supports' object is an instance of 'PrincipalHolder' that apparently can't be QI'd to a nsIXPConnectWrappedNative.

Based on the comment, it sounds like this is the "we can't do much" case, and this is a non-scary situation that's not worthy of spamming up anyone's terminal.  If that's the case, we should just null-check and return, instead of using the spammy NS_ENSURE_TRUE.

Comment 2

[Daniel Holbert [:dholbert]]

Looks like this was last-modified in
 https://hg.mozilla.org/mozilla-central/rev/78fd73ca849e
but that just really added an "if" check (and the explanatory comment quoted above) around an existing QI / NS_ENSURE_TRUE that dates back to 2001 (line 187 here: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/dom/src/base/nsJSUtils.cpp&rev=1.71#187 )

Anyway -- per the explanatory comment, I don't think we need the NS_ENSURE-spam here.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 4

[Daniel Holbert [:dholbert]]

Note that according to bz and Ms2ger (in IRC), do_QueryWrappedNative isn't guaranteed to be null-safe.

(If it were, then we could just completely drop the NS_ENSURE_TRUE, let do_QueryWrappedNative fail, and end up returning null via the "return sgo;" at the end)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 727861 [details] [diff] [review]
fix v1

r=me

Comment 6

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37b1ec12f606

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/37b1ec12f606