Bug 169638 – Web Inspector: Exception when fetching computed styles can break future updates of section

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

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

[Joseph Pecoraro]

<rdar://problem/30588688>

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 304436 [details]
[PATCH] Proposed Fix

[Blaze Burg]

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)?

[Devin Rousso]

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

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 304439 [details]
[PATCH] Proposed Fix

[Devin Rousso]

Comment on attachment 304439 [details]
[PATCH] Proposed Fix

r=me

[WebKit Commit Bot]

Comment on attachment 304439 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 304439

Committed r213961: <http://trac.webkit.org/changeset/213961>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.