1270626 - exception in media query listener causes us to assert due to unhandled ErrorResult

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: match-media-exception.html

I just saw this in the wild while testing dynamic changes to the layout.css.devPixelsPerPx preference, although I didn't catch on what page.

However, it was pretty easy to write a testcase.

If a media query listener throws an exception, we assert (fatally) in a debug build:

Assertion failure: !Failed(), at /home/dbaron/builds/ssd/mozilla-central/obj/firefox-debugopt/dist/include/mozilla/ErrorResult.h:104

#5  0x00007f9177c3f3d0 in <signal handler called> () at /lib/x86_64-linux-gnu/libpthread.so.0
#6  0x00007f9173508376 in mozilla::ErrorResult::~ErrorResult() (this=<optimized out>, __in_chrg=<optimized out>) at /home/dbaron/builds/ssd/mozilla-central/obj/firefox-debugopt/dist/include/mozilla/ErrorResult.h:105
#7  0x00007f9174de061b in nsPresContext::MediaFeatureValuesChanged(nsRestyleHint, nsChangeHint) (this=<optimized out>, aRestyleHint=<optimized out>, aRestyleHint@entry=0, aChangeHint=aChangeHint@entry=(unknown: 0))
    at /home/dbaron/builds/ssd/mozilla-central/mozilla/layout/base/nsPresContext.cpp:1918
#8  0x00007f9174dec5e1 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) (this=<optimized out>) at /home/dbaron/builds/ssd/mozilla-central/mozilla/layout/base/nsPresContext.h:298
#9  0x00007f9174dec5e1 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) (this=this@entry=0x1f512e0, aFlush=...) at /home/dbaron/builds/ssd/mozilla-central/mozilla/layout/base/nsPresShell.cpp:4044
#10 0x00007f9174dec8f2 in PresShell::FlushPendingNotifications(mozFlushType) (this=this@entry=0x1f512e0, aType=<optimized out>) at /home/dbaron/builds/ssd/mozilla-central/mozilla/layout/base/nsPresShell.cpp:3953
#11 0x00007f9174deca70 in PresShell::HandlePostedReflowCallbacks(bool) (this=this@entry=0x1f512e0, aInterruptible=aInterruptible@entry=true) at /home/dbaron/builds/ssd/mozilla-central/mozilla/layout/base/nsPresShell.cpp:3921
#12 0x00007f9174debada in PresShell::DidDoReflow(bool) (this=this@entry=0x1f512e0, aInterruptible=aInterruptible@entry=true) at /home/dbaron/builds/ssd/mozilla-central/mozilla/layout/base/nsPresShell.cpp:9172
#13 0x00007f9174debe10 in PresShell::ResizeReflowIgnoreOverride(int, int) (this=0x1f512e0, aWidth=64620, aHeight=<optimized out>) at /home/dbaron/builds/ssd/mozilla-central/mozilla/layout/base/nsPresShell.cpp:1883
#14 0x00007f9174b14b88 in nsViewManager::DoSetWindowDimensions(int, int) (this=0x1fb5310, aWidth=aWidth@entry=64620, aHeight=aHeight@entry=27660) at /home/dbaron/builds/ssd/mozilla-central/mozilla/view/nsViewManager.cpp:193


This is because the code in nsPresContext::MediaFeatureValuesChanged doesn't do anything with its ErrorResult.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Making the spec updates per bug 1265622 would presumably fix this, but we might want a simpler fix...

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: MozReview Request: Bug 1270626 - Explicitly suppress exceptions from media query listeners, rather than having them assert. r?bzbarsky

I'm acting under the assumption that this is what's closest to what the
code does now, except without asserting in ~ErrorResult.  It also seems
closest to what event listeners will do, both based on examining code
(EventListenerManager::HandleEventSubType, which I'm hoping is the right
code to look at, calls StealNSResult, and then stores it in a member
that's ignored by most callers) and based on testing (for both click
events, and for media query listeners with this patch, the exception
gets reported to the console as an unhandled exception).  That said, I'm
not particularly well versed in the current error handling rules so I
may well be off here.

This code should presumably go away when we change this code to use
EventListeners in bug 1265622, so I don't think there's any spec that
covers this.

Without the patch, the mochitest hits the fatal assertion (after
reporting hitting the expected uncaught exception).  With the
patch the test passes.  (Tested locally.)

Review commit: https://reviewboard.mozilla.org/r/50941/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50941/

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8749428 [details]
MozReview Request: Bug 1270626 - Explicitly suppress exceptions from media query listeners, rather than having them assert.  r?bzbarsky

https://reviewboard.mozilla.org/r/50941/#review47657

::: layout/base/nsPresContext.cpp:1919
(Diff revision 1)
>      if (!notifyList.IsEmpty()) {
>        for (uint32_t i = 0, i_end = notifyList.Length(); i != i_end; ++i) {
>          nsAutoMicroTask mt;
>          MediaQueryList::HandleChangeData &d = notifyList[i];
>          ErrorResult result;
>          d.callback->Call(*d.mql, result);

Just change this to:

    d.callback->Call(*d.mql);
    
and nix `result` altogether.  That will set up an `IgnoredErrorResult` on the stack and call the two-arg version.

r=me

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95e5e81a23509c062bc5b4804bd9662ca25e1d14
Bug 1270626 - Don't leave an unhandled ErrorResult and assert when media query listeners throw exceptions.  r=bzbarsky

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/95e5e81a2350