Bug 169638 - Web Inspector: Exception when fetching computed styles can break future updates of section
Summary: Web Inspector: Exception when fetching computed styles can break future updat...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-03-14 15:37 PDT by Joseph Pecoraro
Modified: 2017-03-14 17:05 PDT (History)
5 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (3.17 KB, patch)
2017-03-14 15:38 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (3.17 KB, patch)
2017-03-14 16:21 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.