1552911 - AddressSanitizer: SEGV /builds/worker/workspace/build/src/obj-firefox/dist/include/nsWrapperCacheInlines.h:14:13 in GetWrapperPreserveColor

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jason Kratzer [:jkratzer]]

Attachment: testcase.html

Testcase found while fuzzing mozilla-central rev 97dae745c1b3. Testcase requires the layout.css.resizeobserver.enabled pref set to true in order to reproduce.

==7886==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000010 (pc 0x7fdded620cee bp 0x7ffe60fda5b0 sp 0x7ffe60fda4c0 T0)
==7886==The signal is caused by a READ memory access.
==7886==Hint: address points to the zero page.
#0 0x7fdded620ced in GetWrapperPreserveColor /builds/worker/workspace/build/src/obj-firefox/dist/include/nsWrapperCacheInlines.h:14:13
#1 0x7fdded620ced in nsWrapperCache::GetWrapper() const /builds/worker/workspace/build/src/obj-firefox/dist/include/nsWrapperCacheInlines.h:27
#2 0x7fddf31a4056 in DoGetOrCreateDOMReflector<mozilla::dom::ResizeObserverSize, mozilla::dom::binding_detail::eWrapIntoContextCompartment> /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:1051:26
#3 0x7fddf31a4056 in GetOrCreateDOMReflector<mozilla::dom::ResizeObserverSize> /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:1126
#4 0x7fddf31a4056 in mozilla::dom::ResizeObserverEntry_Binding::get_borderBoxSize(JSContext*, JS::Handle<JSObject*>, mozilla::dom::ResizeObserverEntry*, JSJitGetterCallArgs) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/ResizeObserverBinding.cpp:918
#5 0x7fddf4fd6801 in bool mozilla::dom::binding_detail::GenericGetter<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3061:13
#6 0x7fddfc8758e0 in CallJSNative /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:443:13
#7 0x7fddfc8758e0 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:535
#8 0x7fddfc87a1d3 in InternalCall /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:590:10
#9 0x7fddfc87a1d3 in Call /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:606
#10 0x7fddfc87a1d3 in js::CallGetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:730
#11 0x7fddfcecefff in CallGetter /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2216:12
#12 0x7fddfcecefff in GetExistingProperty<js::CanGC> /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2268
#13 0x7fddfcecefff in NativeGetPropertyInline<js::CanGC> /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2517
#14 0x7fddfcecefff in js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2554
#15 0x7fddfc8825cd in GetProperty /builds/worker/workspace/build/src/js/src/vm/ObjectOperations-inl.h:117:10
#16 0x7fddfc8825cd in GetProperty /builds/worker/workspace/build/src/js/src/vm/ObjectOperations-inl.h:124
#17 0x7fddfc8825cd in js::GetProperty(JSContext*, JS::Handle<JS::Value>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:4489
#18 0x7fddfc84f5dc in GetPropertyOperation /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:216:10
#19 0x7fddfc84f5dc in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:2769
#20 0x7fddfc83fd58 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:423:10
#21 0x7fddfc876253 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:563:13
#22 0x7fddfc877ed2 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:606:8
#23 0x7fddfd4d68f8 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jsapi.cpp:2655:10
#24 0x7fddf459cbc9 in mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/EventListenerBinding.cpp:52:8
#25 0x7fddf584ce92 in HandleEvent<mozilla::dom::EventTarget > /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/EventListenerBinding.h:66:12
#26 0x7fddf584ce92 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener, mozilla::dom::Event*, mozilla::dom::EventTarget*) /builds/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1038
#27 0x7fddf584ef4e in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) /builds/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1239:17
#28 0x7fddf582f771 in HandleEvent /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/EventListenerManager.h:353:5
#29 0x7fddf582f771 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:349
#30 0x7fddf582d9a6 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:551:16
#31 0x7fddf5834714 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:1047:11
#32 0x7fddf883ca49 in nsDocumentViewer::LoadComplete(nsresult) /builds/worker/workspace/build/src/layout/base/nsDocumentViewer.cpp:1104:7
#33 0x7fddfb6a0c32 in nsDocShell::EndPageLoad(nsIWebProgress, nsIChannel*, nsresult) /builds/worker/workspace/build/src/docshell/base/nsDocShell.cpp:6647:20
#34 0x7fddfb69fcf0 in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /builds/worker/workspace/build/src/docshell/base/nsDocShell.cpp:6447:7
#35 0x7fddfb6a5997 in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /builds/worker/workspace/build/src/docshell/base/nsDocShell.cpp
#36 0x7fddf02157d5 in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:1314:3
#37 0x7fddf02143ca in nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:873:14
#38 0x7fddf020ec10 in nsDocLoader::DocLoaderIsEmpty(bool) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:711:9
#39 0x7fddf0212285 in nsDocLoader::OnStopRequest(nsIRequest*, nsresult) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:599:5
#40 0x7fddf0213f14 in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsresult) /builds/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp
#41 0x7fdded89f171 in mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) /builds/worker/workspace/build/src/netwerk/base/nsLoadGroup.cpp:568:22
#42 0x7fddf1b3ce78 in DoUnblockOnload /builds/worker/workspace/build/src/dom/base/Document.cpp:8031:18
#43 0x7fddf1b3ce78 in mozilla::dom::Document::UnblockOnload(bool) /builds/worker/workspace/build/src/dom/base/Document.cpp:7963
#44 0x7fddf1b3b905 in mozilla::dom::Document::DispatchContentLoadedEvents() /builds/worker/workspace/build/src/dom/base/Document.cpp:5101:3
#45 0x7fddf1c46e8b in applyImpl<mozilla::dom::Document, void (mozilla::dom::Document::)()> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1122:12
#46 0x7fddf1c46e8b in apply<mozilla::dom::Document, void (mozilla::dom::Document::)()> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1128
#47 0x7fddf1c46e8b in mozilla::detail::RunnableMethodImpl<mozilla::dom::Document*, void (mozilla::dom::Document::)(), true, (mozilla::RunnableKind)0>::Run() /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1174
#48 0x7fdded5e88e7 in nsThread::ProcessNextEvent(bool, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1175:14
#49 0x7fdded5f0524 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:486:10
#50 0x7fddee970fdf in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:88:21
#51 0x7fddee846cbe in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
#52 0x7fddee846cbe in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308
#53 0x7fddee846cbe in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290
#54 0x7fddf7f93c83 in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:137:27
#55 0x7fddfc259b60 in nsAppStartup::Run() /builds/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:276:30
#56 0x7fddfc5949e7 in XREMain::XRE_mainRun() /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4555:22
#57 0x7fddfc597404 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4693:8
#58 0x7fddfc598c59 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4774:21
#59 0x55c7efda93da in do_main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:212:22
#60 0x55c7efda93da in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:291
#61 0x7fde11803b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /builds/worker/workspace/build/src/obj-firefox/dist/include/nsWrapperCacheInlines.h:14:13 in GetWrapperPreserveColor
==7886==ABORTING

Comment 1

[Daniel Holbert [:dholbert]]

In my debug build, we abort on the first line here, because value (of type ResizeObserverSize*) is null:

template <class T, GetOrCreateReflectorWrapBehavior wrapBehavior>
MOZ_ALWAYS_INLINE bool DoGetOrCreateDOMReflector(
    JSContext* cx, T* value, JS::Handle<JSObject*> givenProto,
    JS::MutableHandle<JS::Value> rval) {
  MOZ_ASSERT(value);

...because we're trying to read the borderBoxSize from a default-constructed ResizeObserverEntry object, and it's null.

(I assume that actual resizeobservers will produce ResizeObserverEntry objects with non-null borderBoxSize values, but in this case we're constructing one by hand from JS, and it hasn't been given a borderBoxSize value.)

In Chrome, the testcase throws Uncaught TypeError: Illegal constructor for the new ResizeObserverEntry(o1) JS.

Comment 2

[Daniel Holbert [:dholbert]]

It looks like Chrome doesn't have a JS-exposed constructor for ResizeObserverEntry. Perhaps we shouldn't either.

Specifically, they have this IDL for that interface, which has 2 members but no constructor:
https://github.com/chromium/chromium/blob/581330a9f8fa0cd9b3b83def1611740a186a4881/third_party/blink/renderer/core/resize_observer/resize_observer_entry.idl

Compare that to e.g. ResizeObserver itself for which they do have a constructor:
https://github.com/chromium/chromium/blob/581330a9f8fa0cd9b3b83def1611740a186a4881/third_party/blink/renderer/core/resize_observer/resize_observer.idl

The spec-provided WebIDL does require a constructor...

[Exposed=Window, Constructor(Element target)]
interface ResizeObserverEntry {

...but then again the spec-provided WebIDL also has ResizeObservation in it which is only an example and non-normative. So I don't think the spec-provided WebIDL is actually too authoritative/set-in-stone here.

Comment 3

[Daniel Holbert [:dholbert]]

I filed https://github.com/w3c/csswg-drafts/issues/3946 to get the spec updated.

Comment 4

[Boris Chiou [:boris]]

(In reply to Daniel Holbert [:dholbert] from comment #3)

I filed https://github.com/w3c/csswg-drafts/issues/3946 to get the spec updated.

Thanks for filing this issue. Just wait for spec editors' feedback. However, perhaps we should create empty objects when someone calls the exposed APIs if these objects are not initialized:

  DOMRectReadOnly* ContentRect() const { return mContentRect; }
  ResizeObserverSize* BorderBoxSize() const { return mBorderBoxSize; }
  ResizeObserverSize* ContentBoxSize() const { return mContentBoxSize; }

Didn't notice this issue before. :O

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Boris Chiou [:boris] from comment #4)

(In reply to Daniel Holbert [:dholbert] from comment #3)

I filed https://github.com/w3c/csswg-drafts/issues/3946 to get the spec updated.

perhaps we should create empty objects when someone calls the exposed APIs if these objects are not initialized:

Maybe -- though if we remove the JS-exposed constructor, then I think ResizeObserverEntry is only constructed in one spot, and it's in a spot where we have meaningful values for these members:
https://searchfox.org/mozilla-central/rev/a887c90ea9c19a0b5529a1f5fa351929944887ba/dom/base/ResizeObserver.cpp#222-230
...so we wouldn't need dummy values anymore at that point.

If we remove the JS-exposed constructor, then we could perhaps just adjust our internal ResizeObserverEntry constructor to take already_AddRefed<> args for each of the member-variables, and reorder the searchfox-linked-code above accordingly. (And then the constructor could assert that the member-vars are non-null.)

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Boris Chiou [:boris] from comment #4)

Thanks for filing this issue. Just wait for spec editors' feedback.

You probably don't need to bother waiting too long -- given that our implementation is still Nightly-only, and given that this change (removing the JS-exposed constructor) is pretty clearly-sane and matches what Chrome does, I'd say we should just go ahead with removing the JS-exposed constructor at our earliest convenience.

(And if the spec edit ends up being seriously different from what we expect, we could always make a followup change afterwards.)

Comment 7

[Boris Chiou [:boris]]

(In reply to Daniel Holbert [:dholbert] from comment #6)

(In reply to Boris Chiou [:boris] from comment #4)

Thanks for filing this issue. Just wait for spec editors' feedback.

You probably don't need to bother waiting too long -- given that our implementation is still Nightly-only, and given that this change (removing the JS-exposed constructor) is pretty clearly-sane and matches what Chrome does, I'd say we should just go ahead with removing the JS-exposed constructor at our earliest convenience.

(And if the spec edit ends up being seriously different from what we expect, we could always make a followup change afterwards.)

Oh. Actually, I have a patch to fix the crash, but it just makes sure we don't crash based on the current version (which still expose the constructor.) I could update it sooner or later.

Comment 8

[Boris Chiou [:boris]]

Attachment: Bug 1552911 - Drop the constructor of ResizeObserverEntry.

There is a spec issue about should we expose this API:
https://github.com/w3c/csswg-drafts/issues/3946

However, we should make sure it is stable based on the current spec.

Comment 9

[Daniel Holbert [:dholbert]]

FWIW, after a closer look at the spec, it looks like the currently-specced behavior is defined better than I'd thought.
https://github.com/w3c/csswg-drafts/issues/3946#issuecomment-494176266
We're technically supposed to populate the entry with meaningful layout measurements. (which might be 0,0 e.g. in the case of an element that's not part of the DOM & not being rendered, but might also be a real element)

Given that Chrome folks wrote the spec but then never implemented that part, I wonder if it's really useful/necessary though. It seems kind of like getBoxQuads and getBoundingClientRect are better JS APIs for getting roughly the same information that new ResizeObserverEntry would give you.

Comment 10

[Boris Chiou [:boris]]

Attachment: Bug 1552911 - Tweak the internal C++ constructor of ResizeObserverEntry to require aBorderBoxSize and aContentBoxSize.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b9ce11f1880a
Drop the constructor of ResizeObserverEntry. r=dholbert,smaug
https://hg.mozilla.org/integration/autoland/rev/aa35720a2132
Tweak the internal C++ constructor of ResizeObserverEntry to require aBorderBoxSize and aContentBoxSize. r=dholbert

Comment 12

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/b9ce11f1880a
https://hg.mozilla.org/mozilla-central/rev/aa35720a2132