1055722 - "WARNING: NS_ENSURE_TRUE(content) failed: file layout/generic/nsSelection.cpp, line 4030" is way too spammy; makes up ~9% of some mochitest logs

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Daniel Holbert [:dholbert]]

Linux debug mochitest-2 logs are approximately 10% composed of instances of this one warning:
{
WARNING: NS_ENSURE_TRUE(content) failed: file /builds/slave/m-in-l64-d-0000000000000000000/build/layout/generic/nsSelection.cpp, line 4030
}


Sample log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=46282743&tree=Mozilla-Inbound
Ubuntu VM 12.04 x64 mozilla-inbound debug test mochitest-2 on 2014-08-19 10:49:04 PDT for push 39830d7054c6

Line count: 67533
Lines that are this warning: 5971

Comment 1

[Daniel Holbert [:dholbert]]

This NS_ENSURE_TRUE probably just wants to be a null-check-and-early-return, given that it fails this frequently.

It actually used to be a null-check, but we changed that here, for bug 767169:
http://hg.mozilla.org/mozilla-central/diff/61070cc08413/layout/generic/nsSelection.cpp#l1.82

Comment 2

[Daniel Holbert [:dholbert]]

Aryeh, do you remember why this changed to NS_ENSURE_STATE?  And do you have any objection to restoring it to just be a null-check-and-return?

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I don't know what this code is doing, so I can't say.  If it's not reasonable to call this function with an aRange whose start is not in a content node, then probably the caller(s) that are triggering this line should be fixed.  If it's reasonable to call the function and returning early is expected behavior, then it would make sense for it not to warn.

Comment 4

[Arnaud Sourioux [:Six]]

Hi,

I'm interested on working on this.
It looks like code calling Selection::selectFrames() never checks its return value even if this method uses different return values.
I need to know what you choose to replace it.

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

smaug, do you know if this function should be warning if called with a non-content node, or is an early return without warning fine?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

no need to warn, but curious, does aRange->GetStartParent() return document here then?

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a00c8ebbd0e7

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a00c8ebbd0e7