Summary: Our style fetching should be resilient in the face of exceptions. Steps to Reproduce: 1. Introduce an exception inside of `fetchedComputedStyle` or its friends 2. Inspect a page 3. Show Computed Styles sidebar 4. Select a bunch of nodes 5. Switch between nodes => No updates
<rdar://problem/30588688>
This was originally detected with an Augmented JSContext with CSS Styles. In that case certain assumptions (like the computed style must contain certain properties) fails and we get unexpected exceptions. In any case we should be resilient against that.
Created attachment 304436 [details] [PATCH] Proposed Fix
Comment on attachment 304436 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=304436&action=review > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:89 > + promise.resolve(); Shouldn't this reject (and ignore if appropriate where the promise is chained)?
Comment on attachment 304436 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=304436&action=review > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:84 > + return function() { I'm not sure if this will ever happen, but I don't think this will capture `this` correctly. You might want to use an arrow function instead. An example: function wrap() { console.log("outer", this); return function() { console.log("inner", this); } } wrap.call()() // outer - Window // inner - Window wrap.call({})() // outer - {} // inner - Window
Comment on attachment 304436 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=304436&action=review >> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:84 >> + return function() { > > I'm not sure if this will ever happen, but I don't think this will capture `this` correctly. You might want to use an arrow function instead. An example: > > function wrap() { > console.log("outer", this); > return function() { > console.log("inner", this); > } > } > > wrap.call()() > // outer - Window > // inner - Window > > wrap.call({})() > // outer - {} > // inner - Window `this` is never captured. In here, `this` is bound by the caller of wrap. wrap(...).bind(this). Should I make it more explicit? >> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:89 >> + promise.resolve(); > > Shouldn't this reject (and ignore if appropriate where the promise is chained)? This won't trigger the Promise.all below. I can make that include a catch, but I figured it ultimately doesn't matter here.
Created attachment 304439 [details] [PATCH] Proposed Fix
Comment on attachment 304439 [details] [PATCH] Proposed Fix r=me
Comment on attachment 304439 [details] [PATCH] Proposed Fix Clearing flags on attachment: 304439 Committed r213961: <http://trac.webkit.org/changeset/213961>
All reviewed patches have been landed. Closing bug.