Bugzilla

BugBot [:suhaib / :marco/ :calixte]

Comment 2

•

5 years ago

Is there any reason this bug is Mozilla-confidential?

Flags: needinfo?(ksereduck)

Regressed by: 1525372

Updated

•

5 years ago

Keywords: regression

Kimberly Sereduck :kimberlythegeek

Reporter

Comment 3

•

5 years ago

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

Is there any reason this bug is Mozilla-confidential?

(In reply to Kimberly Sereduck :kimberlythegeek from comment #0)

Marking as confidential bug to avoid potentially leaking sensitive information.

Flags: needinfo?(ksereduck)

Assignee

Updated

•

5 years ago

Assignee: nobody → matt.woodrow

Priority: -- → P2

Assignee

Comment 4

•

5 years ago

Attached file Bug 1556441 - Cache the nsIContent in BackgroundClipRenderingObserver construction, so that we can guarantee it's available when we need it. r?jwatt (obsolete) — Details

Comment 5

•

5 years ago

Is it not an invariant that frames are always destroyed before their elements? If that is indeed supposed to hold, do you have any idea how we end up crashing here?

Flags: needinfo?(matt.woodrow)

Comment 6

•

5 years ago

Maybe background propagation is propagating background-clip to the viewport, which has no content node? In that case the patch won't help.

Comment 7

•

5 years ago

Doesn't seem to be the case: data:text/html,<body style="background: green; background-clip: text; color: transparent">Foo.

Oh, so pseudos: https://searchfox.org/mozilla-central/rev/227f5329f75bd8b16c6b146a7414598a420260cb/layout/generic/nsFrame.cpp#841

It should be trivial to find a crasher then.

Comment 8

•

5 years ago

Maybe not as trivial, from a quick try... But I think pseudos is the most reasonable option :)

Comment 9

•

5 years ago

I'm also unclear, on reflection, how introducing BackgroundClipRenderingObserver even fixed bug 1525372. It essentially just makes nsFrame::DidReflow call InvalidateFrame() on itself and its continuations (at the point when INVALIDATE_REFLOW is passed). But aren't we going to invalidate for that reflow anyway...?

Comment 10

•

5 years ago

(In reply to Jonathan Watt [:jwatt] from comment #9)

I'm also unclear, on reflection, how introducing BackgroundClipRenderingObserver even fixed bug 1525372. It essentially just makes nsFrame::DidReflow call InvalidateFrame() on itself and its continuations (at the point when INVALIDATE_REFLOW is passed). But aren't we going to invalidate for that reflow anyway...?

The issue here is that the background is painted by an ancestor of the frame, which may not need invalidation otherwise.

Comment 11

•

5 years ago

FWIW a more robust way of fixing this crash may simply be to remove the StopObserving() call from the BackgroundClipRenderingObserver dtor. We know that in the case of this class the observed and observer frame are the same frame, and that if we're being destroyed it's because our frame is going away. So there's no need to call StopObserving().

Comment 12

•

5 years ago

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)

The issue here is that the background is painted by an ancestor of the frame, which may not need invalidation otherwise.

So nsDisplayBackgroundImage and/or nsDisplayBackgroundColor may be created for an ancestor of the frame with background-clip:text?

Assignee

Comment 13

•

5 years ago

(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)

Maybe not as trivial, from a quick try... But I think pseudos is the most reasonable option :)

The pseudo path you linked was indeed what I believe the problem to be, but I don't have a testcase either.

(In reply to Jonathan Watt [:jwatt] from comment #12)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)

The issue here is that the background is painted by an ancestor of the frame, which may not need invalidation otherwise.

So nsDisplayBackgroundImage and/or nsDisplayBackgroundColor may be created for an ancestor of the frame with background-clip:text?

No, but the 'contents' of that display item (the clipping path) is computed using the style/position of descendants (ideally text descendants, but currently can be any descendants). We need to invalidate the frame with background-clip:text if its children change.

Flags: needinfo?(matt.woodrow)

Comment 14

•

5 years ago

(In reply to Jonathan Watt [:jwatt] from comment #11)

FWIW a more robust way of fixing this crash may simply be to remove the StopObserving() call from the BackgroundClipRenderingObserver dtor. We know that in the case of this class the observed and observer frame are the same frame, and that if we're being destroyed it's because our frame is going away. So there's no need to call StopObserving().

As discussed at the All Hands, I'd prefer to do this since then there's no need to have the extra member or have AddRef/Release.

Flags: needinfo?(matt.woodrow)

Assignee

Comment 15

•

5 years ago

This is just a nullptr crash, no need to be confidential.

Group: mozilla-employee-confidential

Flags: needinfo?(matt.woodrow)

Phabricator Automation

Updated

•

5 years ago

Attachment #9070150 - Attachment is obsolete: true

Natalia Csoregi [:nataliaCs]

Assignee

Comment 16

•

5 years ago

Attached file Bug 1556441 - Don't call StopObservering when destroying a BackgroundClipRenderingObserver since we know the frame is going away anyway. r?jwatt — Details

Pulsebot

Comment 17

•

5 years ago

Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d2d42067781b
Don't call StopObservering when destroying a BackgroundClipRenderingObserver since we know the frame is going away anyway. r=jwatt

Comment 18

•

5 years ago

Backed out for failures on SVGObserverUtils.cpp and nsISupportsImpl.h

backout: https://hg.mozilla.org/integration/autoland/rev/c905c2290c3bb5c52a1a5b19118d0a044bfc9b4c

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d2d42067781b566820701a07797f2772ad560439&group_state=expanded&selectedJob=253652114

failure log: * https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=253652130&repo=autoland&lineNumber=22352
* https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=253652114&repo=autoland&lineNumber=22455

[task 2019-06-27T04:45:38.744Z] 04:45:38 ERROR - /builds/worker/workspace/build/src/layout/svg/SVGObserverUtils.cpp:563:1: error: static_assert failed due to requirement '!mozilla::IsDestructible<mozilla::BackgroundClipRenderingObserver>::value' "Reference-counted class BackgroundClipRenderingObserver should not have a public destructor. Make this class's destructor non-public"
[task 2019-06-27T04:45:38.747Z] 04:45:38 INFO - NS_IMPL_ISUPPORTS(BackgroundClipRenderingObserver, nsIMutationObserver)
[task 2019-06-27T04:45:38.747Z] 04:45:38 INFO - ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2019-06-27T04:45:38.747Z] 04:45:38 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/nsISupportsImpl.h:1199:3: note: expanded from macro 'NS_IMPL_ISUPPORTS'
[task 2019-06-27T04:45:38.747Z] 04:45:38 INFO - NS_IMPL_ADDREF(aClass)
[task 2019-06-27T04:45:38.747Z] 04:45:38 INFO - ^~~~~~~~~~~~~~~~~~~~~~
[task 2019-06-27T04:45:38.747Z] 04:45:38 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/nsISupportsImpl.h:684:32: note: expanded from macro 'NS_IMPL_ADDREF'
[task 2019-06-27T04:45:38.747Z] 04:45:38 INFO - #define NS_IMPL_ADDREF(_class) NS_IMPL_NAMED_ADDREF(_class, #_class)
[task 2019-06-27T04:45:38.747Z] 04:45:38 INFO - ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2019-06-27T04:45:38.747Z] 04:45:38 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/nsISupportsImpl.h:671:5: note: expanded from macro 'NS_IMPL_NAMED_ADDREF'
[task 2019-06-27T04:45:38.747Z] 04:45:38 INFO - MOZ_ASSERT_TYPE_OK_FOR_REFCOUNTING(_class)
[task 2019-06-27T04:45:38.747Z] 04:45:38 INFO - ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2019-06-27T04:45:38.747Z] 04:45:38 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/nsISupportsImpl.h:32:3: note: expanded from macro 'MOZ_ASSERT_TYPE_OK_FOR_REFCOUNTING'
[task 2019-06-27T04:45:38.747Z] 04:45:38 INFO - static_assert(!mozilla::IsDestructible<X>::value,
[task 2019-06-27T04:45:38.747Z] 04:45:38 INFO - ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2019-06-27T04:45:38.747Z] 04:45:38 INFO - 1 error generated.
[task 2019-06-27T04:45:38.747Z] 04:45:38 INFO - /builds/worker/workspace/build/src/config/rules.mk:801: recipe for target 'Unified_cpp_layout_svg0.o' failed
[task 2019-06-27T04:45:38.747Z] 04:45:38 ERROR - make[4]: *** [Unified_cpp_layout_svg0.o] Error 1

[task 2019-06-27T04:45:34.573Z] 04:45:34 ERROR - /builds/worker/workspace/build/src/obj-firefox/dist/include/nsISupportsImpl.h:32:3: error: static assertion failed: Reference-counted class BackgroundClipRenderingObserver should not have a public destructor. Make this class's destructor non-public
[task 2019-06-27T04:45:34.573Z] 04:45:34 INFO - static_assert(!mozilla::IsDestructible<X>::value,
[task 2019-06-27T04:45:34.573Z] 04:45:34 INFO - ^
[task 2019-06-27T04:45:34.573Z] 04:45:34 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/nsISupportsImpl.h:671:5: note: in expansion of macro 'MOZ_ASSERT_TYPE_OK_FOR_REFCOUNTING'
[task 2019-06-27T04:45:34.573Z] 04:45:34 INFO - MOZ_ASSERT_TYPE_OK_FOR_REFCOUNTING(_class)
[task 2019-06-27T04:45:34.573Z] 04:45:34 INFO - ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2019-06-27T04:45:34.573Z] 04:45:34 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/nsISupportsImpl.h:684:32: note: in expansion of macro 'NS_IMPL_NAMED_ADDREF'
[task 2019-06-27T04:45:34.573Z] 04:45:34 INFO - #define NS_IMPL_ADDREF(_class) NS_IMPL_NAMED_ADDREF(_class, #_class)
[task 2019-06-27T04:45:34.573Z] 04:45:34 INFO - ^~~~~~~~~~~~~~~~~~~~
[task 2019-06-27T04:45:34.573Z] 04:45:34 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/nsISupportsImpl.h:1199:3: note: in expansion of macro 'NS_IMPL_ADDREF'
[task 2019-06-27T04:45:34.573Z] 04:45:34 INFO - NS_IMPL_ADDREF(aClass)
[task 2019-06-27T04:45:34.573Z] 04:45:34 INFO - ^~~~~~~~~~~~~~
[task 2019-06-27T04:45:34.573Z] 04:45:34 INFO - /builds/worker/workspace/build/src/layout/svg/SVGObserverUtils.cpp:563:1: note: in expansion of macro 'NS_IMPL_ISUPPORTS'
[task 2019-06-27T04:45:34.573Z] 04:45:34 INFO - NS_IMPL_ISUPPORTS(BackgroundClipRenderingObserver, nsIMutationObserver)
[task 2019-06-27T04:45:34.573Z] 04:45:34 INFO - ^
[task 2019-06-27T04:45:34.573Z] 04:45:34 INFO - /builds/worker/workspace/build/src/config/rules.mk:801: recipe for target 'Unified_cpp_layout_svg0.o' failed
[task 2019-06-27T04:45:34.573Z] 04:45:34 ERROR - make[4]: *** [Unified_cpp_layout_svg0.o] Error 1

Flags: needinfo?(matt.woodrow)

Pulsebot

Comment 19

•

5 years ago

Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ae0ec3d4ed20
Don't call StopObservering when destroying a BackgroundClipRenderingObserver since we know the frame is going away anyway. r=jwatt