974127 - nsContentUtils::IsUserFocusIgnored can loop infinitely

Status: RESOLVED FIXED

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

Description

[Jeff Muizelaar [:jrmuizel]]

I ran into this running reftests. aNode is an HTML element, QueryInterface(aNode) to nsIMozBrowserFrame returns null, and aNode->OwnerDoc()->GetWindow() also returns null and so we loop forever.

Comment 1

[(no longer active)]

Which reftest was that?  This could manifest itself as an intermittent test timeout.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

This was during one of the bmp refrests. I don't remember which one. Afaik the tests are not really interesting from a browser point of view.

Comment 3

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

Attachment: patch

Comment 4

[(no longer active)]

Comment on attachment 8378579 [details] [diff] [review]
patch

Review of attachment 8378579 [details] [diff] [review]:
-----------------------------------------------------------------

Can you please document what the loop termination condition here is?  It's not very clear what this code is trying to do, but the fix makes sense.  Thanks!

Comment 5

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

It is not clear? Just a document which doesn't have a window (anymore).

Comment 6

[(no longer active)]

(In reply to comment #5)
> It is not clear? Just a document which doesn't have a window (anymore).

Yes, well, I was talking about a higher level description.  What you said is basically converting the code to English.  :-)

Comment 7

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

Ok, still don't understand what kind of comment you want.

Comment 8

[(no longer active)]

(In reply to comment #7)
> Ok, still don't understand what kind of comment you want.

So here is my understanding of this code.

We try to see if the current node is a mozbrowser iframe.  If not, we try to get it from the window object of the node's owner document.  Now, it's not clear to me what exactly happens if the window's frame is not a mozbrowser iframe.  According to the code we should try to get the window again.  Does that mean that we're walking up the chain of the hosting frames until we find a mozbrowser iframe?  Or do we stop after checking at that point and bail out of the loop?

(Note that even though the purpose of this loop may be completely clear to you, it's not to me, and comments are meant to make things like this clear to the readers of the code who are not necessarily as familiar with the code as others.)

Comment 9

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

The method has already a comment
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3b246449a75

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[bhavana bajaj [:bajaj]]

Blocking as this is needed by 1005391 and Ni :fabrice to help with uplift on 1.3T branch.

Comment 12

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/31c2f2808051