Created attachment 8886776[details]
reduced testcase
Hi all.
The following was identified on the following Mozillazine thread and from this I have built up a simplified testcase. Loading this testcase with the preference layout.css.servo.enabled set to true will lead to a crash.
http://forums.mozillazine.org/viewtopic.php?p=14756121#p14756121
Note that the style sheet does not exist at all and it isn't required to trigger this crash. If the stylesheet link, table or the embed elements are removed then the crash is no longer reproducible.
If you require any further information please let me know.

I cannot reproduce this crash with either the url provided in the forum post or the reduced testcase.
A recent crash report bp-3dbe4fe5-ffc4-463d-a7c2-9b9260170715 contains MOZ_CRASH Reason saying "called `Option::unwrap()` on a `None` value", and looking at that function, the only place which seems to be "borrow.as_ref().unwrap()" for handling InheritFromBodyQuirk.
That partially makes sense, because the test case includes a table, which is where the quirk applies. But the quirk should only be active in quirks mode, while the reduced testcase has "<!DOCTYPE HTML" so it is presumably in standard mode.
The embed seems to be a flash, and I have no idea how that involves here.

So I decided to test this on Servo's nightly developer preview and it seems to work fine. This seems to somehow only be related to the Servo CSS setting within Firefox. It doesn't occur with Servo on its own.

So, I think I know what's happening here, although I have no idea what is the proper way to fix it.
This panic itself is because that, when we are resolving color property (on the table element) which uses InheritFromBodyQuirk, the body element doesn't have style data assigned yet.
The body element not having style data is because we are still building the content tree, so we haven't even started the initial restyle.
The reason why we want to resolve color property before the initial restyle is that, there is a load policy implemented in js (installed by addons) which we want to invoke when we see the <embed> element during content tree building. And for the js policy, we want to hand out the element in question to script. For handing out the element, we need to GetBindingURL in Element::WrapObject (this part is something I haven't fully understood).
In GetBindingURL, we call into nsComputedDOMStyle::GetStyleContextNoFlush which resolves style data recursively.
There are two questions I'd like to ask first:
First, are those elements (object, embed, and applet) still relevant that we need to install binding before handing them to js? The code is from bug 804950 (5yrs ago) and the comment says we check them because they "may have a plugin-related overlay". However, nowadays we have disabled all plugins except Flash. Do we still need to check them, then? If no, I guess we can just exclude them from that check, so that we don't need to touch this code in this way at all.
Second, if we cannot exclude them from the check, this means we can be resolving element style before the initial restyle. I don't think this is a situation we consider before, is it? There could be random places where we assume element style data should exist before styling. The computation code crashes here is one example of that. It may not be a big deal, but we need to keep in mind that this can happen.
For this specific case, I think a simple workaround is just to avoid unwrap, but instead go to the fallback route in case there is no style data with body element. Should be an easy fix. I'll try to construct a test as well.

ni? bz for this question:
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #13)
> First, are those elements (object, embed, and applet) still relevant that we
> need to install binding before handing them to js? The code is from bug
> 804950 (5yrs ago) and the comment says we check them because they "may have
> a plugin-related overlay". However, nowadays we have disabled all plugins
> except Flash. Do we still need to check them, then? If no, I guess we can
> just exclude them from that check, so that we don't need to touch this code
> in this way at all.

> However, nowadays we have disabled all plugins except Flash.
Doesn't matter. The point is that these are objects that can have bindings installed by our chrome (click to play and whatnot) that need to be set up properly. Or at least that used to be the case.
Whether it's still the case I can't tell you, but that's unrelated to the set of plugins we support.
> this means we can be resolving element style before the initial restyle
I think that's true no matter what you do with the plugin bits. For example, you can have a page like this:
<!DOCTYPE html>
<script>
alert(getComputedStyle(document.head).color);
</script>
That said, my attempt to reproduce a crash with a table in quirks mode like so:
<script>
var b = document.createElement("body");
document.documentElement.appendChild(b);
var t = document.createElement("table");
b.appendChild(t);
alert(getComputedStyle(t).color);
</script>
doesn't seem to crash for me...

(In reply to Boris Zbarsky [:bz] from comment #18)
> I think that's true no matter what you do with the plugin bits. For
> example, you can have a page like this:
>
> <!DOCTYPE html>
> <script>
> alert(getComputedStyle(document.head).color);
> </script>
IIUC, getComputedStyle flushes all style (and also create frames?) so any existing element should get the style data assigned I suppose.