33915 2010-01-20 11:29:45 -0800 Web Inspector: JavaScript Error in DOMAgent.js:375 (_attributesUpdated) 2010-02-08 15:45:12 -0800 1 1 1 Unclassified WebKit Web Inspector (Deprecated) 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 bweinstein knorton bweinstein joepeck keishi pfeldman pmuellr rik timothy oldest_to_newest 182874 0 bweinstein 2010-01-20 11:29:45 -0800 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]. 183187 1 47117 knorton 2010-01-21 06:53:24 -0800 Created attachment 47117 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. 183548 2 47117 mjs 2010-01-22 01:56:49 -0800 Comment on attachment 47117 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. 183557 3 47117 pfeldman 2010-01-22 02:06:58 -0800 Comment on attachment 47117 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. 183607 4 knorton 2010-01-22 05:20:41 -0800 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. 183617 5 pfeldman 2010-01-22 05:57:56 -0800 (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. 183693 6 47210 knorton 2010-01-22 10:28:25 -0800 Created attachment 47210 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. 183704 7 knorton 2010-01-22 11:04:30 -0800 Mac failure was bogus, this patch is good for review. 183871 8 knorton 2010-01-22 16:38:32 -0800 http://trac.webkit.org/changeset/53736 47117 2010-01-21 06:53:24 -0800 2010-01-22 10:28:25 -0800 Proposed fix. Issue33915.diff text/plain 2054 knorton ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg MjhmNzRlOC4uMjEzZTk5ZiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNyBAQAorMjAxMC0wMS0yMSAgS2VsbHkgTm9ydG9u ICA8a25vcnRvbkBnb29nbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIFdlYiBJbnNwZWN0b3I6IEphdmFTY3JpcHQgRXJyb3IgaW4gRE9NQWdl bnQuanM6Mzc1IChfYXR0cmlidXRlc1VwZGF0ZWQpCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD0zMzkxNQorCisgICAgICAgIEVycm9ycyB3ZXJlIGNhdXNl ZCBieSB0aGUgZmFjdCB0aGF0IEVsZW1lbnQ6OmF0dHJpYnV0ZXMgY2FsbHMgRWxlbWVudDo6c2V0 QXR0cmlidXRlIHRvCisgICAgICAgIHN5bmNocm9uaXplZCB0aGUgc3R5bGVBdHRyLiBUaGUgZml4 IGlzIHRvIHNpbXBseSBjaGVjayB0aGUgc3luY2hyb25pemluZyBzdHlsZSBhdHRyaWJ1dGUKKyAg ICAgICAgZmxhZy4KKworICAgICAgICAqIGRvbS9FbGVtZW50LmNwcDoKKyAgICAgICAgKFdlYkNv cmU6OkVsZW1lbnQ6OnNldEF0dHJpYnV0ZSk6IENoZWNrZWQgZm9yIGNhc2Ugd2hlcmUgc3R5bGVB dHRyIGlzIGJlaW5nIHN5bmNocm9uaXplZC4KKwogMjAxMC0wMS0yMSAgU3RldmUgQmxvY2sgIDxz dGV2ZWJsb2NrQGdvb2dsZS5jb20+CiAKICAgICAgICAgVW5yZXZpZXdlZCBzcGVjdWxhdGl2ZSBi dWlsZCBmaXggZm9yIFdpbmRvd3MuCmRpZmYgLS1naXQgYS9XZWJDb3JlL2RvbS9FbGVtZW50LmNw cCBiL1dlYkNvcmUvZG9tL0VsZW1lbnQuY3BwCmluZGV4IDIzNGJlYzQuLmYyN2JkYmYgMTAwNjQ0 Ci0tLSBhL1dlYkNvcmUvZG9tL0VsZW1lbnQuY3BwCisrKyBiL1dlYkNvcmUvZG9tL0VsZW1lbnQu Y3BwCkBAIC01MzIsOCArNTMyLDEwIEBAIHZvaWQgRWxlbWVudDo6c2V0QXR0cmlidXRlKGNvbnN0 IEF0b21pY1N0cmluZyYgbmFtZSwgY29uc3QgQXRvbWljU3RyaW5nJiB2YWx1ZSwKIAogI2lmIEVO QUJMRShJTlNQRUNUT1IpCiAgICAgaWYgKFBhZ2UqIHBhZ2UgPSBkb2N1bWVudCgpLT5wYWdlKCkp IHsKLSAgICAgICAgaWYgKEluc3BlY3RvckNvbnRyb2xsZXIqIGluc3BlY3RvckNvbnRyb2xsZXIg PSBwYWdlLT5pbnNwZWN0b3JDb250cm9sbGVyKCkpCi0gICAgICAgICAgICBpbnNwZWN0b3JDb250 cm9sbGVyLT5kaWRNb2RpZnlET01BdHRyKHRoaXMpOworICAgICAgICBpZiAoSW5zcGVjdG9yQ29u dHJvbGxlciogaW5zcGVjdG9yQ29udHJvbGxlciA9IHBhZ2UtPmluc3BlY3RvckNvbnRyb2xsZXIo KSkgeworICAgICAgICAgICAgaWYgKCFtX3N5bmNocm9uaXppbmdTdHlsZUF0dHJpYnV0ZSkKKyAg ICAgICAgICAgICAgICBpbnNwZWN0b3JDb250cm9sbGVyLT5kaWRNb2RpZnlET01BdHRyKHRoaXMp OworICAgICAgICB9CiAgICAgfQogI2VuZGlmCiB9CkBAIC01NTksOCArNTYxLDEwIEBAIHZvaWQg RWxlbWVudDo6c2V0QXR0cmlidXRlKGNvbnN0IFF1YWxpZmllZE5hbWUmIG5hbWUsIGNvbnN0IEF0 b21pY1N0cmluZyYgdmFsdWUsCiAKICNpZiBFTkFCTEUoSU5TUEVDVE9SKQogICAgIGlmIChQYWdl KiBwYWdlID0gZG9jdW1lbnQoKS0+cGFnZSgpKSB7Ci0gICAgICAgIGlmIChJbnNwZWN0b3JDb250 cm9sbGVyKiBpbnNwZWN0b3JDb250cm9sbGVyID0gcGFnZS0+aW5zcGVjdG9yQ29udHJvbGxlcigp KQotICAgICAgICAgICAgaW5zcGVjdG9yQ29udHJvbGxlci0+ZGlkTW9kaWZ5RE9NQXR0cih0aGlz KTsKKyAgICAgICAgaWYgKEluc3BlY3RvckNvbnRyb2xsZXIqIGluc3BlY3RvckNvbnRyb2xsZXIg PSBwYWdlLT5pbnNwZWN0b3JDb250cm9sbGVyKCkpIHsKKyAgICAgICAgICAgIGlmICghbV9zeW5j aHJvbml6aW5nU3R5bGVBdHRyaWJ1dGUpCisgICAgICAgICAgICAgICAgaW5zcGVjdG9yQ29udHJv bGxlci0+ZGlkTW9kaWZ5RE9NQXR0cih0aGlzKTsKKyAgICAgICAgfQogICAgIH0KICNlbmRpZgog fQo= 47210 2010-01-22 10:28:25 -0800 2010-02-08 15:45:12 -0800 Updated patch (unchanged, trying again) Issue33915.diff text/plain 2044 knorton ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg NDRlZjU0Zi4uZTlmYjNmYiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNyBAQAorMjAxMC0wMS0yMSAgS2VsbHkgTm9ydG9u ICA8a25vcnRvbkBnb29nbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIFdlYiBJbnNwZWN0b3I6IEphdmFTY3JpcHQgRXJyb3IgaW4gRE9NQWdl bnQuanM6Mzc1IChfYXR0cmlidXRlc1VwZGF0ZWQpCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD0zMzkxNQorCisgICAgICAgIEVycm9ycyB3ZXJlIGNhdXNl ZCBieSB0aGUgZmFjdCB0aGF0IEVsZW1lbnQ6OmF0dHJpYnV0ZXMgY2FsbHMgRWxlbWVudDo6c2V0 QXR0cmlidXRlIHRvCisgICAgICAgIHN5bmNocm9uaXplZCB0aGUgc3R5bGVBdHRyLiBUaGUgZml4 IGlzIHRvIHNpbXBseSBjaGVjayB0aGUgc3luY2hyb25pemluZyBzdHlsZSBhdHRyaWJ1dGUKKyAg ICAgICAgZmxhZy4KKworICAgICAgICAqIGRvbS9FbGVtZW50LmNwcDoKKyAgICAgICAgKFdlYkNv cmU6OkVsZW1lbnQ6OnNldEF0dHJpYnV0ZSk6IENoZWNrZWQgZm9yIGNhc2Ugd2hlcmUgc3R5bGVB dHRyIGlzIGJlaW5nIHN5bmNocm9uaXplZC4KKwogMjAxMC0wMS0yMiAgR2lyaXNoIFJhbWFrcmlz aG5hbiAgPGdpcmlzaEBmb3J3YXJkYmlhcy5pbj4KIAogICAgICAgICBSZXZpZXdlZCBieSBTaW1v biBIYXVzbWFubi4KZGlmZiAtLWdpdCBhL1dlYkNvcmUvZG9tL0VsZW1lbnQuY3BwIGIvV2ViQ29y ZS9kb20vRWxlbWVudC5jcHAKaW5kZXggNTQ0NTMyZC4uMjc5MjZmYiAxMDA2NDQKLS0tIGEvV2Vi Q29yZS9kb20vRWxlbWVudC5jcHAKKysrIGIvV2ViQ29yZS9kb20vRWxlbWVudC5jcHAKQEAgLTUz Miw4ICs1MzIsMTAgQEAgdm9pZCBFbGVtZW50OjpzZXRBdHRyaWJ1dGUoY29uc3QgQXRvbWljU3Ry aW5nJiBuYW1lLCBjb25zdCBBdG9taWNTdHJpbmcmIHZhbHVlLAogCiAjaWYgRU5BQkxFKElOU1BF Q1RPUikKICAgICBpZiAoUGFnZSogcGFnZSA9IGRvY3VtZW50KCktPnBhZ2UoKSkgewotICAgICAg ICBpZiAoSW5zcGVjdG9yQ29udHJvbGxlciogaW5zcGVjdG9yQ29udHJvbGxlciA9IHBhZ2UtPmlu c3BlY3RvckNvbnRyb2xsZXIoKSkKLSAgICAgICAgICAgIGluc3BlY3RvckNvbnRyb2xsZXItPmRp ZE1vZGlmeURPTUF0dHIodGhpcyk7CisgICAgICAgIGlmIChJbnNwZWN0b3JDb250cm9sbGVyKiBp bnNwZWN0b3JDb250cm9sbGVyID0gcGFnZS0+aW5zcGVjdG9yQ29udHJvbGxlcigpKSB7CisgICAg ICAgICAgICBpZiAoIW1fc3luY2hyb25pemluZ1N0eWxlQXR0cmlidXRlKQorICAgICAgICAgICAg ICAgIGluc3BlY3RvckNvbnRyb2xsZXItPmRpZE1vZGlmeURPTUF0dHIodGhpcyk7CisgICAgICAg IH0KICAgICB9CiAjZW5kaWYKIH0KQEAgLTU1OSw4ICs1NjEsMTAgQEAgdm9pZCBFbGVtZW50Ojpz ZXRBdHRyaWJ1dGUoY29uc3QgUXVhbGlmaWVkTmFtZSYgbmFtZSwgY29uc3QgQXRvbWljU3RyaW5n JiB2YWx1ZSwKIAogI2lmIEVOQUJMRShJTlNQRUNUT1IpCiAgICAgaWYgKFBhZ2UqIHBhZ2UgPSBk b2N1bWVudCgpLT5wYWdlKCkpIHsKLSAgICAgICAgaWYgKEluc3BlY3RvckNvbnRyb2xsZXIqIGlu c3BlY3RvckNvbnRyb2xsZXIgPSBwYWdlLT5pbnNwZWN0b3JDb250cm9sbGVyKCkpCi0gICAgICAg ICAgICBpbnNwZWN0b3JDb250cm9sbGVyLT5kaWRNb2RpZnlET01BdHRyKHRoaXMpOworICAgICAg ICBpZiAoSW5zcGVjdG9yQ29udHJvbGxlciogaW5zcGVjdG9yQ29udHJvbGxlciA9IHBhZ2UtPmlu c3BlY3RvckNvbnRyb2xsZXIoKSkgeworICAgICAgICAgICAgaWYgKCFtX3N5bmNocm9uaXppbmdT dHlsZUF0dHJpYnV0ZSkKKyAgICAgICAgICAgICAgICBpbnNwZWN0b3JDb250cm9sbGVyLT5kaWRN b2RpZnlET01BdHRyKHRoaXMpOworICAgICAgICB9CiAgICAgfQogI2VuZGlmCiB9Cg==