Bug 169638

Summary: Web Inspector: Exception when fetching computed styles can break future updates of section
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, drousso, inspector-bugzilla-changes, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Proposed Fix
none
[PATCH] Proposed Fix none

Description Joseph Pecoraro 2017-03-14 15:37:06 PDT
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
Comment 1 Joseph Pecoraro 2017-03-14 15:37:16 PDT
<rdar://problem/30588688>
Comment 2 Joseph Pecoraro 2017-03-14 15:38:03 PDT
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.
Comment 3 Joseph Pecoraro 2017-03-14 15:38:56 PDT
Created attachment 304436 [details]
[PATCH] Proposed Fix
Comment 4 BJ Burg 2017-03-14 15:42:21 PDT
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 5 Devin Rousso 2017-03-14 15:47:50 PDT
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 6 Joseph Pecoraro 2017-03-14 16:04:19 PDT
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.
Comment 7 Joseph Pecoraro 2017-03-14 16:21:15 PDT
Created attachment 304439 [details]
[PATCH] Proposed Fix
Comment 8 Devin Rousso 2017-03-14 16:33:04 PDT
Comment on attachment 304439 [details]
[PATCH] Proposed Fix

r=me
Comment 9 WebKit Commit Bot 2017-03-14 17:05:30 PDT
Comment on attachment 304439 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 304439

Committed r213961: <http://trac.webkit.org/changeset/213961>
Comment 10 WebKit Commit Bot 2017-03-14 17:05:34 PDT
All reviewed patches have been landed.  Closing bug.