Closed Bug 1292275 Opened 8 years ago Closed 8 years ago

Stylo crash in ServoStyleSet::AddDocStyleSheet in layout/reftests/font-inflation/list-1.html

Categories

(Core :: Layout, defect, P1)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: cpeterson, Assigned: mbrubeck)

References

Details

(Keywords: crash, Whiteboard: [stylo:m2])

Attachments

(1 file)

Bug 1292275 - Stylo: Fix crash after failed stylesheet load.
58 bytes, text/x-review-board-request
heycam
: review+
Details 
To reproduce, run `./mach reftest --disable-e10s <filename>` or replace <filename> with `layout/reftest/reftest.list`.

TEST-UNEXPECTED-FAIL | file:///home/shinglyu/workspace/stylo/gecko-dev/layout/reftests/font-inflation/list-1.html | application terminated with exit code 11
REFTEST PROCESS-CRASH | file:///home/shinglyu/workspace/stylo/gecko-dev/layout/reftests/font-inflation/list-1.html | application crashed [None]
--
We call mozilla::ServoStyleSet::AddDocStyleSheet with a ServoStyleSheet for whom RawSheet() returns null.
Priority: -- → P1
The stack trace is:

[8633] ###!!! ASSERTION: Bad loading table: 'mSheets->mLoadingDatas.Get(&key, &loadingData) && loadingData == aLoadData', file /home/mbrubeck/src/mozilla-central/layout/style/Loader.cpp, line 1879
#01: mozilla::css::Loader::DoSheetComplete(mozilla::css::SheetLoadData*, nsresult, nsTArray<RefPtr<mozilla::css::SheetLoadData> >&) (/home/mbrubeck/src/mozilla-central/layout/style/Loader.cpp:1877 (discriminator 7))
#02: mozilla::css::Loader::SheetComplete(mozilla::css::SheetLoadData*, nsresult) (/home/mbrubeck/src/mozilla-central/layout/style/Loader.cpp:1825)
#03: mozilla::css::SheetLoadData::OnStreamComplete(nsIUnicharStreamLoader*, nsISupports*, nsresult, nsAString_internal const&) (/home/mbrubeck/src/mozilla-central/layout/style/Loader.cpp:878)
#04: nsUnicharStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) (/home/mbrubeck/src/mozilla-central/netwerk/base/nsUnicharStreamLoader.cpp:100)
#05: mozilla::net::nsStreamListenerTee::OnStopRequest(nsIRequest*, nsISupports*, nsresult) (/home/mbrubeck/src/mozilla-central/netwerk/base/nsStreamListenerTee.cpp:51)
#06: mozilla::net::nsHttpChannel::OnStopRequest(nsIRequest*, nsISupports*, nsresult) (/home/mbrubeck/src/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:6767)
#07: nsInputStreamPump::OnStateStop() (/home/mbrubeck/src/mozilla-central/netwerk/base/nsInputStreamPump.cpp:715)
#08: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (/home/mbrubeck/src/mozilla-central/netwerk/base/nsInputStreamPump.cpp:433)
#09: nsInputStreamReadyEvent::Run() (/home/mbrubeck/src/mozilla-central/xpcom/io/nsStreamUtils.cpp:97)
#10: nsThread::ProcessNextEvent(bool, bool*) (/home/mbrubeck/src/mozilla-central/xpcom/threads/nsThread.cpp:1082)
#11: NS_ProcessNextEvent(nsIThread*, bool) (/home/mbrubeck/src/mozilla-central/xpcom/glue/nsThreadUtils.cpp:290)
#12: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/mbrubeck/src/mozilla-central/ipc/glue/MessagePump.cpp:96)
#13: MessageLoop::RunInternal() (/home/mbrubeck/src/mozilla-central/ipc/chromium/src/base/message_loop.cc:233)
#14: MessageLoop::RunHandler() (/home/mbrubeck/src/mozilla-central/ipc/chromium/src/base/message_loop.cc:226)
#15: MessageLoop::Run() (/home/mbrubeck/src/mozilla-central/ipc/chromium/src/base/message_loop.cc:204)
#16: nsBaseAppShell::Run() (/home/mbrubeck/src/mozilla-central/widget/nsBaseAppShell.cpp:158)
#17: nsAppStartup::Run() (/home/mbrubeck/src/mozilla-central/toolkit/components/startup/nsAppStartup.cpp:283)
#18: XREMain::XRE_mainRun() (/home/mbrubeck/src/mozilla-central/toolkit/xre/nsAppRunner.cpp:4450)
#19: XREMain::XRE_main(int, char**, nsXREAppData const*) (/home/mbrubeck/src/mozilla-central/toolkit/xre/nsAppRunner.cpp:4583)
#20: XRE_main (/home/mbrubeck/src/mozilla-central/toolkit/xre/nsAppRunner.cpp:4674)
#21: do_main(int, char**, char**, nsIFile*) (/home/mbrubeck/src/mozilla-central/browser/app/nsBrowserApp.cpp:282)
#22: main (/home/mbrubeck/src/mozilla-central/browser/app/nsBrowserApp.cpp:415)
#23: __libc_start_main (/build/glibc-Px319Y/glibc-2.24/csu/../csu/libc-start.c:325)
#24: _start (/home/mbrubeck/src/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/firefox)
Assignee: nobody → mbrubeck
The bug is triggered by this line in the reftest, which links to a non-existent "ahem.css":

<link rel="stylesheet" type="text/css" href="ahem.css" />

I can reproduce this assertion failure using the following HTML file, where "404" is not a path to a CSS file:

<link rel="stylesheet" href="404">
Note: The assertion in comment 1 is in debug-only code, so it might be masking a different crash in non-debug builds.
The underlying bug behind the assertion is that Loader::LoadSheet is called twice for the same URI, but the second call does not "glom on" to the existing load because the relevant check is in a Gecko-only code path.  As a result, the SheetLoadData inserted into Loader::mLoadingDatas by the first load is overwritten by the second load.
Attachment #8804911 - Flags: review?(xidorn+moz)
This patch fixes the crash from comment 0 which occurs in non-debug builds.

Debug builds still hit the assertion in comment 1.  I'm working on fixing that now and will post that fix separately.
Attachment #8804911 - Flags: review?(xidorn+moz) → review?(cam)
I'm not familiar with CSS Loader. Redirect to heycam, although he seems to be quite busy currently... Hmm...
Depends on: 1314045
Filed bug 1314045 for the debug-only assertion.
Comment on attachment 8804911 [details]
Bug 1292275 - Stylo: Fix crash after failed stylesheet load.

https://reviewboard.mozilla.org/r/88744/#review89562

::: layout/style/Loader.cpp:1889
(Diff revision 1)
>                                           aLoadData->mLoaderPrincipal,
>                                           aLoadData->mSheet->GetCORSMode(),
>                                           aLoadData->mSheet->GetReferrerPolicy());
>  #ifdef DEBUG
>        SheetLoadData *loadingData;
> +      mSheets->mLoadingDatas.Get(&key, &loadingData);

Why do we need this line?

::: layout/style/ServoStyleSheet.cpp:93
(Diff revision 1)
> +  return ParseSheet(EmptyString(),
> +                    aSheetURI,
> +                    aBaseURI,
> +                    aSheetPrincipal,
> +                    aLineNumber);

This works, though all the unnecessary work to pass through the URIs, principal and line number when none of them will be used when parsing the empty string is not that great.  A new function on the Stylesheet impl to create an empty sheet, and an FFI function to call it, would probably be better.  WDYT?
Comment on attachment 8804911 [details]
Bug 1292275 - Stylo: Fix crash after failed stylesheet load.

https://reviewboard.mozilla.org/r/88744/#review91074

r=me with the below.  Thanks for fixing this!

::: servo/ports/geckolib/glue.rs:172
(Diff revision 2)
> +    let extra_data = ParserContextExtraData {
> +        base: None,
> +        referrer: None,
> +        principal: None,
> +    };

You can use ParserContextExtraData::default() (or Default::default(), whichever is more idiomatic, I'm not sure) instead of this.
Attachment #8804911 - Flags: review?(cam) → review+
Servo part submitted as https://github.com/servo/servo/pull/14142
Pushed by mbrubeck@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/069a62008441
Stylo: Fix crash after failed stylesheet load. r=heycam
https://hg.mozilla.org/mozilla-central/rev/069a62008441
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: