Bug 18595 – REGRESSION (r20766): Setting display:none on an iframe causes the ownerDocument to freeze

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Arnaud LB]

Setting style.display='none' on an iframe element in the context of the iframe (after a mouse event in the iframe) causes the owner document to freeze all user actions:

- Clicking on a link does nothing
- Clicking on a submit button does nothing
- etc.

How to repeat: http://s3.amazonaws.com/arnaud.lb/webkit-iframe-bug.html

[Alexey Proskuryakov]

Confirmed with r32287.

[Alexey Proskuryakov]

Created attachment 56957 [details]
proposed fix

[Darin Adler]

Comment on attachment 56957 [details]
proposed fix

> +    if (m_capturingMouseEventsNode && (m_capturingMouseEventsNode->hasTagName(iframeTag) || m_capturingMouseEventsNode->hasTagName(frameTag)))

I think this would read a lot better with an isFrame-type helper function, even if the implementation does the same hasTagName tests. What about objectTag? Can't it contain subframes too?

r=me but please consider both comments

[Alexey Proskuryakov]

Created attachment 57043 [details]
proposed fix

Yes, objects and embeds can also hold capturing frames.

[Darin Adler]

Comment on attachment 57043 [details]
proposed fix

Fix now covers <object> and <embed>, but the test case doesn't cover those.

> +        RefPtr and is initizlized automatically.

This has a typo: "initizlized".

>      , m_resizeLayer(0)
> -    , m_capturingMouseEventsNode(0)
>      , m_clickCount(0)

m_shouldResetCapturingMouseEventsNode ought to be initialized to make memory debugging tools happy if nothing else.

> -    void setCapturingMouseEventsNode(PassRefPtr<Node>);
> +    void setCapturingMouseEventsNode(PassRefPtr<Node>); // A caller is respponsible for resetting capturing node to 0.

This has a typo: "respponsible".

I also find this comment a bit confusing. In the context of this patch I know what it means.

I think we should have a comment in the one place where we set m_shouldResetCapturingMouseEventsNode saying when the capturing will automatically end. In fact, I think the name of the boolean data member should communicate when the capturing will automatically get reset, not just the fact that it will be reset.

review+ but please fix at least the uninitialized boolean

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/60186>.

(In reply to comment #5)
> In fact, I think the name of the boolean data member should communicate when the capturing will automatically get reset, not just the fact that it will be reset.

Renamed to m_eventHandlerWillResetCapturingMouseEventsNode.

> review+ but please fix at least the uninitialized boolean

What is the problem memory debugging tools could have with this? I added the initialization, but I don't see how it can be necessary. We previously had issues with tools that detected non-errors, but that was when we were reading uninitialized values and not using them, which is not what happens here.

[Darin Adler]

(In reply to comment #6)
> What is the problem memory debugging tools could have with this? I added the initialization, but I don't see how it can be necessary. We previously had issues with tools that detected non-errors, but that was when we were reading uninitialized values and not using them, which is not what happens here.

There's no chance of a mismatched mouse release event without a mouse press?

[Alexey Proskuryakov]

Given that frames capture events, I don't see how that could happen.