stylo: crash when page involves an embed and external stylesheet

VERIFIED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
P1
major
VERIFIED FIXED
9 days ago
a day ago

People

(Reporter: sciguyryan, Assigned: xidorn)

Tracking

(Blocks: 1 bug, {crash, testcase})

Trunk
mozilla56
x86_64
Windows 10
crash, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox54 unaffected, firefox55 unaffected, firefox56 fixed, firefox-esr52 unaffected)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 days ago
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.
(Reporter)

Updated

9 days ago
Flags: needinfo?(xidorn+moz)
(Reporter)

Comment 1

9 days ago
https://crash-stats.mozilla.com/report/index/ad52f379-a54e-42a8-aaee-695a90170715 is an example of the crash that is created - the stack is [@ style::values::specified::color::{{impl}}::to_computed_value ]

Since I can't see the editable crash stack list in the
Keywords: crash, testcase
(Reporter)

Comment 2

9 days ago
The following is a link to the crash stack as produced by this bug: https://crash-stats.mozilla.com/report/index/ad52f379-a54e-42a8-aaee-695a90170715

As I cannot seem to find the correct place to input the crash stack on this bug, I will post it here for quick reference: [@ style::values::specified::color::{{impl}}::to_computed_value ]
(Assignee)

Updated

9 days ago
Blocks: 1375906
Summary: style: crash when page involves an embed and external stylesheet → stylo: crash when page involves an embed and external stylesheet
(Assignee)

Comment 3

9 days ago
Thanks for reporting. I'll investigate it next Monday.
Crash Signature: [@ style::values::specified::color::{{impl}}::to_computed_value ]
(Assignee)

Comment 4

8 days ago
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.
Flags: needinfo?(xidorn+moz)
Priority: -- → P2
Crash Signature: [@ style::values::specified::color::{{impl}}::to_computed_value ] → [@ style::values::specified::color::{{impl}}::to_computed_value ] [@ alloc::oom::default_oom_handler | style::values::specified::color::{{impl}}::to_computed_value ]
Priority: P2 → P1
(Reporter)

Comment 5

7 days ago
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.

Comment 6

7 days ago
I can reproduce this on new profile only with some extensions installed:
    https://addons.mozilla.org/firefox/addon/privacy-badger17/ or
    https://www.eff.org/files/https-everywhere-test/index.html
(Reporter)

Comment 7

7 days ago
(In reply to John from comment #6)
> I can reproduce this on new profile only with some extensions installed:
>     https://addons.mozilla.org/firefox/addon/privacy-badger17/ or
>     https://www.eff.org/files/https-everywhere-test/index.html

I have both of those installed so it may help narrow down what is causing this!

Comment 8

7 days ago
I also crash many times with Stylo enabled.  The only site that consistently crashes for me when stylo is enabled is when my add-on https://addons.mozilla.org/en-US/firefox/addon/zoom-page-we/ is also enabled.

Comment 9

7 days ago
Forgot to give you the site that crashes:

http://www.alternate-tools.com/pages/c_mathsolver.php?lang=ENG
Xidorn, can you reproduce this crash with any of the new URLs?
Assignee: nobody → xidorn+moz
Flags: needinfo?(xidorn+moz)
As mentioned, the crashes seem to be related to particular WE add-ons.  If you have no WE add-ons or perhaps ones that don't cause the problem you might not get the crash.
(Assignee)

Comment 12

6 days ago
Yep, I can reproduce this issue with the given add-on installed and the attached testcase. I'll investigate.
Flags: needinfo?(xidorn+moz)
(Assignee)

Comment 13

6 days ago
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.
(Assignee)

Comment 14

6 days ago
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.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 15

6 days ago
Also it is not yet clear to me why the extra style is necessary either...
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
> 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...
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 19

5 days ago
(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.
Ah, the key is the no-flush style computation that the binding stuff does, ok.

Comment 21

4 days ago
mozreview-review
Comment on attachment 8887850 [details]
Don't panic when body element don't have style data.

https://reviewboard.mozilla.org/r/158770/#review164514

::: servo/components/style/values/specified/color.rs:273
(Diff revision 1)
>                  use dom::TElement;
>                  use gecko::wrapper::GeckoElement;
>                  use gecko_bindings::bindings::Gecko_GetBody;
>                  let pres_context = _context.device.pres_context();
> -                let body = unsafe {
> -                    Gecko_GetBody(pres_context)
> +                let body = unsafe { Gecko_GetBody(pres_context) }.map(|body| GeckoElement(body));
> +                let borrow = body.as_ref().and_then(|wrap| wrap.borrow_data());

Nit: maybe "data" or "body_data" rather than "borrow"?
Attachment #8887850 - Flags: review?(cam) → review+

Comment 22

4 days ago
mozreview-review
Comment on attachment 8887851 [details]
Bug 1381233 - Add test for this bug.

https://reviewboard.mozilla.org/r/158772/#review164518

I didn't read the test closely, so rs=me assuming it fails without your patch.  Thanks for going to the effort of writing this non-trivial test, too.
Attachment #8887851 - Flags: review?(cam) → review+
(Assignee)

Comment 23

4 days ago
(In reply to Cameron McCormack (:heycam) from comment #22)
> Comment on attachment 8887851 [details]
> Bug 1381233 - Add test for this bug.
> 
> https://reviewboard.mozilla.org/r/158772/#review164518
> 
> I didn't read the test closely, so rs=me assuming it fails without your
> patch.  Thanks for going to the effort of writing this non-trivial test, too.

The majority of the test is copied from dom/base/test/test_bug375314.html actually :)

And yeah, I tested that it does crash without the patch.
Duplicate of this bug: 1382710
The test in bug 1382710 is quite neat actually, let's consider landing that instead?
(Assignee)

Comment 26

4 days ago
Yeah, I guess we can land both.
(Assignee)

Comment 27

4 days ago
Servo PR: servo/servo#17799
Comment hidden (mozreview-request)
(Assignee)

Updated

4 days ago
Attachment #8887850 - Attachment is obsolete: true

Comment 29

4 days ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 8bc7b954ec4b -d 6ed469370daa: rebasing 408380:8bc7b954ec4b "Bug 1381233 - Add test for this bug. r=heycam" (tip)
merging layout/style/crashtests/crashtests.list
merging layout/style/test/mochitest.ini
warning: conflicts while merging layout/style/crashtests/crashtests.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 30

4 days ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1ba89b033552
Add test for this bug. r=heycam
(Reporter)

Comment 31

3 days ago
I can confirm that the patch from autoland fixes this issue. I'll leave this one open until it lands on central.

Thanks for working on this :)

Comment 32

3 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1ba89b033552
Status: NEW → RESOLVED
Last Resolved: 3 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Reporter)

Updated

2 days ago
Status: RESOLVED → VERIFIED
status-firefox54: --- → unaffected
status-firefox55: --- → unaffected
status-firefox-esr52: --- → unaffected
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.