Closed Bug 1357716 Opened 7 years ago Closed 7 years ago

Stylo: Rule objects should have non-null sheet

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jryans, Assigned: mbrubeck)

References

Details

Attachments

(1 file)

Bug 1357716 - stylo: Don't return uninitialized pointers from GetCSSStyleRules.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
DevTools expects CSS rule objects to have a non-null `parentStyleSheet` (which is backed by `mSheet`, but currently Stylo objects default to null.

Many DevTools tests[1] currently fail with errors because of this:

Full message: TypeError: sheet is undefined
Full stack: exports.isContentStylesheet@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/inspector/css-logic.js:95:3
_getElementRules@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/styles.js:576:23
_getAllElementRules@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/styles.js:530:5
PageStyleActor<.getApplied<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/styles.js:449:30
_run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:311:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:273:3
asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:247:14
handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1082:19
onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1759:15
receiveMessage@chrome://marionette/content/server.js -> resource://devtools/shared/transport/transport.js:761:7

[1]: https://treeherder.mozilla.org/logviewer.html#?job_id=92644574&repo=try&lineNumber=3688
This seems to affect most inspector tests, but I specifically copied this stack from "devtools/client/shared/test/browser_telemetry_button_responsive.js".
I would consider this pretty high priority, since it currently blocks most inspector tests from proceeding further, so we can't tell if there are more errors hiding beyond this.
Priority: -- → P2
I'd like to elevate this bug to P1 per comment 2. Bobby, WDYT?
Flags: needinfo?(bobbyholley)
Priority: P2 → P1
Yep, agreed. Xidorn, among you, bradwerth, and jryans, who is best positioned to take this?
Flags: needinfo?(bobbyholley) → needinfo?(xidorn+moz)
I'll have a look. I don't expect this to happen... keep ni?.
It seems like we set it already? 

https://dxr.mozilla.org/mozilla-central/rev/e17cbb839dd225a2da7e5d5bec43cf94e11749d8/layout/style/ServoCSSRuleList.cpp#106


I tried testing this by just inspecting `document.styleSheets[0]`,  but I got a crash when obtaining it:

Assertion failure: !aIID.Equals((nsISupports::COMTypeInfo<nsISupports, void>::kIID)), at /Users/manishearth/mozilla/hydragyrum/layout/style/StyleSheet.cpp:123
#01: mozilla::ServoStyleSheet::QueryInterface(nsID const&, void**)[/Users/manishearth/mozilla/hydragyrum/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2cc82d5]
#02: nsCOMPtr_base::assign_from_qi(nsQueryInterface, nsID const&)[/Users/manishearth/mozilla/hydragyrum/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7b865]
#03: xpc::UnwrapReflectorToISupports(JSObject*)[/Users/manishearth/mozilla/hydragyrum/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0xbe9ccd]
#04: xpc::HasInstance(JSContext*, JS::Handle<JSObject*>, nsID const*, bool*)[/Users/manishearth/mozilla/hydragyrum/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0xba0981]
#05: nsJSIID::HasInstance(nsIXPConnectWrappedNative*, JSContext*, JSObject*, JS::Handle<JS::Value>, bool*, bool*)[/Users/manishearth/mozilla/hydragyrum/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0xba0d0e]
#06: non-virtual thunk to nsJSIID::HasInstance(nsIXPConnectWrappedNative*, JSContext*, JSObject*, JS::Handle<JS::Value>, bool*, bool*)[/Users/manishearth/mozilla/hydragyrum/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0xba0dc1]
#07: XPC_WN_Helper_HasInstance(JSContext*, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>, bool*)[/Users/manishearth/mozilla/hydragyrum/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0xbdef28]
#08: js::HasInstance(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, bool*)[/Users/manishearth/mozilla/hydragyrum/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x424d429]
#09: Interpret(JSContext*, js::RunState&)[/Users/manishearth/mozilla/hydragyrum/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x424802a]
#10: js::RunScript(JSContext*, js::RunState&)[/Users/manishearth/mozilla/hydragyrum/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x423a83e]
#11: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)[/Users/manishearth/mozilla/hydragyrum/obj-x86_64-apple-darwin15.6.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x424b453]
#12: js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandl
So that code (_getElementRules) calls DOMUtils.getCSSStyleRules, which doesn't go through ServoCSSRuleList.

It instead constructs fresh CSS rules, and *does not* set the parent style sheet there.
`inDOMUtils::GetCSSStyleRules` calls `ServoStyleSheet::GetCssRulesInternal` which does call `SetStyleSheet` on the returned rules.  The actual problem I found was failure to check the return value of `nsDataHashtable::Get` on this line:

https://dxr.mozilla.org/mozilla-central/rev/85e5d15c3169/layout/inspector/inDOMUtils.cpp#303

This `Get` is returning false for some raw rules, and failure to check the return value is causing garbage pointers to be returned from this function.
Assignee: nobody → mbrubeck
Status: NEW → ASSIGNED
Sounds like it is a dupe of bug 1359217 then.
Here's a band-aid patch that skips rules that aren't found.  This prevents `GetCSSStyleRules` from returning bad pointers, which fixes the devtools crash above.  I'll leave bug 1359217 open to track the real fix for the missing ServoStyleRules.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=959130f14e75161cec3da7e9c5499cb86fb7c544
Comment on attachment 8869246 [details]
Bug 1357716 - stylo: Don't return uninitialized pointers from GetCSSStyleRules.

https://reviewboard.mozilla.org/r/140810/#review144312

::: layout/inspector/inDOMUtils.cpp:302
(Diff revision 1)
>      }
>  
>      // Find matching rules in the table.
>      for (size_t j = 0; j < rawRuleCount; j++) {
>        const RawServoStyleRule* rawRule = rawRuleList.ElementAt(j);
> -      ServoStyleRule* rule;
> +      ServoStyleRule* rule = 0;

You probably should use `nullptr` for initializing null pointer.
Attachment #8869246 - Flags: review?(xidorn+moz) → review+
Blocks: 1359217
Pushed by mbrubeck@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2afa6edea6d0
stylo: Don't return uninitialized pointers from GetCSSStyleRules. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/2afa6edea6d0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: needinfo?(xidorn+moz)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: