Closed Bug 1381233 Opened 7 years ago Closed 7 years ago

stylo: crash when page involves an embed and external stylesheet

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Platform:
x86_64
Windows 10
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed

People

(Reporter: sciguyryan, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(2 files, 1 obsolete file)

reduced testcase
390 bytes, text/html
Details
Bug 1381233 - Add test for this bug.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Attached file 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.
Flags: needinfo?(xidorn+moz)
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
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 ]
Blocks: stylo-site-issues
Summary: style: crash when page involves an embed and external stylesheet → stylo: crash when page involves an embed and external stylesheet
Thanks for reporting. I'll investigate it next Monday.
Crash Signature: [@ style::values::specified::color::{{impl}}::to_computed_value ]
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
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.
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
(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!
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.
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.
Yep, I can reproduce this issue with the given add-on installed and the attached testcase. I'll investigate.
Flags: needinfo?(xidorn+moz)
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.
Flags: needinfo?(bzbarsky)
Also it is not yet clear to me why the extra style is necessary either...
> 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)
(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 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 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+
(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.
The test in bug 1382710 is quite neat actually, let's consider landing that instead?
Yeah, I guess we can land both.
Attachment #8887850 - Attachment is obsolete: true
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)
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 :)
https://hg.mozilla.org/mozilla-central/rev/1ba89b033552
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: