Bug 18595 - REGRESSION (r20766): Setting display:none on an iframe causes the ownerDocument to freeze
Summary: REGRESSION (r20766): Setting display:none on an iframe causes the ownerDocume...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Alexey Proskuryakov
URL: http://s3.amazonaws.com/arnaud.lb/web...
Keywords:
Depends on:
Blocks:
 
Reported: 2008-04-18 13:12 PDT by Arnaud LB
Modified: 2010-05-25 14:53 PDT (History)
2 users (show)

See Also:


Attachments
proposed fix (7.67 KB, patch)
2010-05-24 18:54 PDT, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff
proposed fix (10.19 KB, patch)
2010-05-25 14:00 PDT, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arnaud LB 2008-04-18 13:12:58 PDT
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
Comment 1 Alexey Proskuryakov 2008-04-21 08:27:39 PDT
Confirmed with r32287.
Comment 2 Alexey Proskuryakov 2010-05-24 18:54:03 PDT
Created attachment 56957 [details]
proposed fix
Comment 3 Darin Adler 2010-05-25 09:40:38 PDT
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
Comment 4 Alexey Proskuryakov 2010-05-25 14:00:40 PDT
Created attachment 57043 [details]
proposed fix

Yes, objects and embeds can also hold capturing frames.
Comment 5 Darin Adler 2010-05-25 14:10:15 PDT
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
Comment 6 Alexey Proskuryakov 2010-05-25 14:31:53 PDT
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.
Comment 7 Darin Adler 2010-05-25 14:46:45 PDT
(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?
Comment 8 Alexey Proskuryakov 2010-05-25 14:53:08 PDT
Given that frames capture events, I don't see how that could happen.