Summary: | REGRESSION (r20766): Setting display:none on an iframe causes the ownerDocument to freeze | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Arnaud LB <arnaud.lb> | ||||||
Component: | WebCore Misc. | Assignee: | Alexey Proskuryakov <ap> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Major | CC: | ap, darin | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
URL: | http://s3.amazonaws.com/arnaud.lb/webkit-iframe-bug.html | ||||||||
Attachments: |
|
Description
Arnaud LB
2008-04-18 13:12:58 PDT
Created attachment 56957 [details]
proposed fix
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 Created attachment 57043 [details]
proposed fix
Yes, objects and embeds can also hold capturing frames.
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 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. (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? Given that frames capture events, I don't see how that could happen. |