Bug 33915 - Web Inspector: JavaScript Error in DOMAgent.js:375 (_attributesUpdated)
Summary: Web Inspector: JavaScript Error in DOMAgent.js:375 (_attributesUpdated)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Kelly Norton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-20 11:29 PST by Brian Weinstein
Modified: 2010-02-08 15:45 PST (History)
7 users (show)

See Also:


Attachments
Proposed fix. (2.01 KB, patch)
2010-01-21 06:53 PST, Kelly Norton
pfeldman: review+
Details | Formatted Diff | Diff
Updated patch (unchanged, trying again) (2.00 KB, patch)
2010-01-22 10:28 PST, Kelly Norton
pfeldman: review+
eric: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Weinstein 2010-01-20 11:29:45 PST
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].
Comment 1 Kelly Norton 2010-01-21 06:53:24 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 2 Maciej Stachowiak 2010-01-22 01:56:49 PST
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 3 Pavel Feldman 2010-01-22 02:06:58 PST
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.
Comment 4 Kelly Norton 2010-01-22 05:20:41 PST
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.
Comment 5 Pavel Feldman 2010-01-22 05:57:56 PST
(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.
Comment 6 Kelly Norton 2010-01-22 10:28:25 PST
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.
Comment 7 Kelly Norton 2010-01-22 11:04:30 PST
Mac failure was bogus, this patch is good for review.
Comment 8 Kelly Norton 2010-01-22 16:38:32 PST
http://trac.webkit.org/changeset/53736