| Summary: | Web Inspector: JavaScript Error in DOMAgent.js:375 (_attributesUpdated) | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brian Weinstein <bweinstein> | ||||||
| Component: | Web Inspector (Deprecated) | Assignee: | Kelly Norton <knorton> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | bweinstein, joepeck, keishi, pfeldman, pmuellr, rik, timothy | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
|
Description
Brian Weinstein
2010-01-20 11:29:45 PST
Created attachment 47117 [details]
Proposed fix.
The problem is due to the special treatment of the styleAttr. When InspectorDOMAgent calls Element::attributes to build a list of attributes, it ends up calling setAttribute for the styleAttr. This shows up in the InspectorDOMAgent as a spurious Attr modification. The fix is to ignore all attr modifications when m_synchronizingStyleAttribute is set.
Comment on attachment 47117 [details]
Proposed fix.
The EWS seems to be saying that this breaks the build on mac. So r- for that, unless you know this is a false positive for some reason.
Comment on attachment 47117 [details]
Proposed fix.
What exactly doesn't InspectorDOMAgent like? If it is a general issue such as "re-entering agent's code synchronously when building script structures for nodes", I'd rather fix it in InspectorDOMAgent itself. Just mute all the node-related mutation callbacks while building script objects.
I think this is a false positive, since I tested the change on mac. But I will double check. Pavel: The problem is that there is special handling of styleAttr in Element. It uses setAttribute to synchronize its own internal state whenever you ask for attributes. InspectorDOMAgent asks for the attributes of nodes causing setAttribute to be called for the style attribute. The nice thing is that Element actually keeps a flag to indicate when it is doing internal synchronization. So it makes sense to not notify InspectorController when internal state is being synchronized. It would work to put a flag in InspectorDOMAgent, but we would be creating a different flag to track the same thing when Element should not have called InspectorController in the first place (since it knows the setAttribute is not legitimate). I don't think this one is a general re-entrency issue since it is reasonable to expect that calling Element::attributes should not cause attribute mutations. (In reply to comment #4) > I think this is a false positive, since I tested the change on mac. But I will > double check. > > Pavel: > The problem is that there is special handling of styleAttr in Element. It uses > setAttribute to synchronize its own internal state whenever you ask for > attributes. InspectorDOMAgent asks for the attributes of nodes causing > setAttribute to be called for the style attribute. The nice thing is that > Element actually keeps a flag to indicate when it is doing internal > synchronization. So it makes sense to not notify InspectorController when > internal state is being synchronized. It would work to put a flag in > InspectorDOMAgent, but we would be creating a different flag to track the same > thing when Element should not have called InspectorController in the first > place (since it knows the setAttribute is not legitimate). I don't think this > one is a general re-entrency issue since it is reasonable to expect that > calling Element::attributes should not cause attribute mutations. Ok, this sounds good provided that styleAttr is the only of a kind. I was afraid that there could be more active getters like this in operations like attributes, childNodes, etc. Having a single flag in DOM agent would cover all the cases that it is not ready for then. Created attachment 47210 [details]
Updated patch (unchanged, trying again)
I've tried to reproduce the mac build break locally and I've been unable. So, I'm going to resubmit this patch and see if the mac build goes green this time. If not, I'll track down the right people on irc to get the logs from the builder to see what is going wrong.
Mac failure was bogus, this patch is good for review. |