Bug 33915 – Web Inspector: JavaScript Error in DOMAgent.js:375 (_attributesUpdated)

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Brian Weinstein]

I am getting a JavaScript error when I inspect the inspector, even when I haven't interacted with it.

STEPS TO REPRODUCE:
- Go to www.google.com
- Open the inspector [1]
- Open the inspector [2] of the first inspector [1]

There will be a JavaScript error in inspector[2].

[Kelly Norton]

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.

[Maciej Stachowiak]

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.

[Pavel Feldman]

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.

[Kelly Norton]

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.

[Pavel Feldman]

(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.

[Kelly Norton]

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.

[Kelly Norton]

Mac failure was bogus, this patch is good for review.

[Kelly Norton]

http://trac.webkit.org/changeset/53736