Bug 50109 – Web Inspector: crash upon inspecting user style sheet.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Feldman]

Patch to follow.

[Pavel Feldman]

Created attachment 74929 [details]
[PATCH] Proposed change.

[Brian Weinstein]

Comment on attachment 74929 [details]
[PATCH] Proposed change.

View in context: https://bugs.webkit.org/attachment.cgi?id=74929&action=review

> WebCore/ChangeLog:7
> +

Is there a reason resourceStyleSheetText crashes for user style sheets? A little more information here would be nice.

> WebCore/inspector/InspectorStyleSheet.cpp:990
> +    if (m_origin == "user" || m_origin == "user-agent")
> +        return false;

It looks like canBind() in InspectorStyleSheet.h checks for "userAgent". Is there a difference between "user-agent" and "userAgent"? Should these checks match up? It would probably be nice to have those strings in constants as well.

[Alexander Pavlov (apavlov)]

(In reply to comment #2)
> (From update of attachment 74929 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74929&action=review
> > WebCore/inspector/InspectorStyleSheet.cpp:990
> > +    if (m_origin == "user" || m_origin == "user-agent")
> > +        return false;
> 
> It looks like canBind() in InspectorStyleSheet.h checks for "userAgent". Is there a difference between "user-agent" and "userAgent"? Should these checks match up? It would probably be nice to have those strings in constants as well.

Yes, these two should match up. Pavel, can you please fix the occurrence to be "user-agent" (as required by CSSStyleModel.js)? Global constants would be really nice but it is quite tedious to set them up (as class static fields are not allowed in WebKit). Would be great if you could introduce them, though.

[Alexander Pavlov (apavlov)]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 74929 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=74929&action=review
> > > WebCore/inspector/InspectorStyleSheet.cpp:990
> > > +    if (m_origin == "user" || m_origin == "user-agent")
> > > +        return false;
> > 
> > It looks like canBind() in InspectorStyleSheet.h checks for "userAgent". Is there a difference between "user-agent" and "userAgent"? Should these checks match up? It would probably be nice to have those strings in constants as well.
> 
> Yes, these two should match up. Pavel, can you please fix the occurrence to be "user-agent" (as required by CSSStyleModel.js)? Global constants would be really nice but it is quite tedious to set them up (as class static fields are not allowed in WebKit). Would be great if you could introduce them, though.

On the second thought I figured that this would make user-agent styles editable, which will break the CSS editing experience (all editing operations will fail for these styles). To fix this, we should either support editing of styles not backed by source code or implement retrieval of user-agent stylesheet text, as we discussed last Friday offline (the latter is preferable, of course).

[WebKit Commit Bot]

Comment on attachment 74929 [details]
[PATCH] Proposed change.

Clearing flags on attachment: 74929

Committed r72811: <http://trac.webkit.org/changeset/72811>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brian Weinstein]

I'm confused why this was marked r+ and cq+ when there were clear objections to the patch raised in the bug.

[Brian Weinstein]

(In reply to comment #7)
> I'm confused why this was marked r+ and cq+ when there were clear objections to the patch raised in the bug.

Unless I missed some discussion offline that was relevant to the change being correct, I'm still not sure why there is an example of "userAgent" and "user-agent" in the .h and .cpp files.

[Pavel Feldman]

(In reply to comment #8)
> (In reply to comment #7)
> > I'm confused why this was marked r+ and cq+ when there were clear objections to the patch raised in the bug.
> 
> Unless I missed some discussion offline that was relevant to the change being correct, I'm still not sure why there is an example of "userAgent" and "user-agent" in the .h and .cpp files.

Hey, sorry about that. This change was fixing a nasty crasher, so I decided to give it a go (it is #1 crasher in the chromium nightly, i suppose same would be reported from webkit nightlies). The idea was that the part that you noticed was immediately followed up on by Alexander. This did not happen though due to the misunderstanding. We will follow up immediately. Sorry again.