Closed Bug 1604498 Opened 5 years ago Closed 1 year ago

Assertion failure: !mTable (Tear-off objects remain in hashtable at shutdown.), at /builds/worker/workspace/build/src/dom/svg/SVGAttrTearoffTable.h:29

Categories

(Core :: SVG, defect, P5)

defect

Tracking

()

RESOLVED FIXED
115 Branch
Tracking Status
firefox-esr102 --- wontfix
firefox73 --- wontfix
firefox112 --- wontfix
firefox113 --- wontfix
firefox114 --- wontfix
firefox115 --- fixed

People

(Reporter: jkratzer, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, pernosco, testcase, Whiteboard: [fuzzblocker])

Attachments

(4 files, 1 obsolete file)

Attached file testcase.html (obsolete) —

Testcase found while fuzzing mozilla-central rev 83fc8cf83221. Testcase must be served via a local webserver in order to reproduce. Furthermore, testcase may require a few attempts to trigger the assertion.

Flags: in-testsuite?

The priority flag is not set for this bug.
:TYLin, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(aethanyc)

I cannot reproduce this on my local m-c debug build with the testcase served via a local webserver.

Jason, is there any preferences required to set to reproduce this?

Flags: needinfo?(aethanyc) → needinfo?(jkratzer)
Attached file prefs-default-e10s.js
Flags: needinfo?(jkratzer)

:TYLin, I'm not sure exactly which pref is required but it reproduces for me consistently using the prefs attached here.

The priority flag is not set for this bug.
:heycam, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(cam)

Thanks! I can reproduce with the prefs attached in comment 4 by

python -m ffpuppet obj-firefox/dist/bin/firefox -p ~/Downloads/prefs-default-e10s.js -d -u http://localhost:8000/1604498.html --rr -l log

The minumum prefs that I can produce are (I can only reporduce for about 70% of the time, not always).

user_pref("datareporting.healthreport.service.enabled", false);
user_pref("datareporting.healthreport.service.firstRun", false);
user_pref("datareporting.healthreport.uploadEnabled", false);
user_pref("datareporting.policy.firstRunURL", "");
user_pref("dom.allow_scripts_to_close_windows", true);

The full stack is like:

#0  0x00007f70a3d5b4a7 in mozilla::SVGAttrTearoffTable<mozilla::SVGAnimatedLength, mozilla::dom::DOMSVGAnimatedLength>::~SVGAttrTearoffTable() (this=0x7f70ad6d6960 <mozilla::sSVGAnimatedLengthTearoffTable>)
    at /home/aethanyc/Projects/gecko/dom/svg/SVGAttrTearoffTable.h:29
#1  0x00007f70b75d3041 in __run_exit_handlers (status=0, listp=0x7f70b797b718 <__exit_funcs>, run_list_atexit=run_list_atexit@entry=true, run_dtors=run_dtors@entry=true) at exit.c:108
#2  0x00007f70b75d313a in __GI_exit (status=<optimized out>) at exit.c:139
#3  0x00007f70b75b1b9e in __libc_start_main (main=0x55baac7a01f0 <main(int, char**, char**)>, argc=16, argv=0x7ffec9b1aad8, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7ffec9b1aac8)
    at ../csu/libc-start.c:344
#4  0x000055baac7a002a in _start ()

So this looks like a shutdown issue, and the assertion only fired in debug build. P5 for now.

Flags: needinfo?(cam)
Priority: -- → P5
Whiteboard: [fuzzblocker]

Adding an updated testcase.

Testcase found while fuzzing mozilla-central rev bc1d41e88ae3 (built with: --enable-debug --enable-fuzzing).

Testcase can be reproduced using the following commands:

$ pip install fuzzfetch grizzly-framework
$ python -m fuzzfetch --build bc1d41e88ae3 --debug --fuzzing -n firefox
$ python -m grizzly.replay ./firefox/firefox testcase.html
Assertion failure: !mTable (Tear-off objects remain in hashtable at shutdown.), at /dom/svg/SVGAttrTearoffTable.h:30

    ==4125725==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fdda6ad60b5 bp 0x7fff75fce4c0 sp 0x7fff75fce4c0 T4125725)
    ==4125725==The signal is caused by a WRITE memory access.
    ==4125725==Hint: address points to the zero page.
        #0 0x7fdda6ad60b5 in mozilla::SVGAttrTearoffTable<mozilla::SVGAnimatedTransformList, mozilla::dom::DOMSVGAnimatedTransformList>::~SVGAttrTearoffTable() /dom/svg/SVGAttrTearoffTable.h:30:5
        #1 0x7fddb8ee88a6 in __run_exit_handlers /build/glibc-SzIz7B/glibc-2.31/stdlib/exit.c:108:8
        #2 0x7fddb8ee8a5f in exit /build/glibc-SzIz7B/glibc-2.31/stdlib/exit.c:139:3
        #3 0x7fddb8ec6089 in __libc_start_main /build/glibc-SzIz7B/glibc-2.31/csu/../csu/libc-start.c:342:3
        #4 0x55cf8fad6fec in _start (/home/jkratzer/builds/mc-debug/firefox-bin+0x15fec) (BuildId: cfa516c894c505553cab0e07ae8acf4fdb5aac53)
    
    UndefinedBehaviorSanitizer can not provide additional info.
    SUMMARY: UndefinedBehaviorSanitizer: SEGV /dom/svg/SVGAttrTearoffTable.h:30:5 in mozilla::SVGAttrTearoffTable<mozilla::SVGAnimatedTransformList, mozilla::dom::DOMSVGAnimatedTransformList>::~SVGAttrTearoffTable()
    ==4125725==ABORTING
Attached file testcase.html
Attachment #9116399 - Attachment is obsolete: true
Attachment #9288033 - Attachment description: Testcase for comment 49 → testcase.html

This bug prevents fuzzing from making progress; however, it has low severity. It is important for fuzz blocker bugs to be addressed in a timely manner (see here why?).
:jwatt, could you increase the severity?

For more information, please visit auto_nag documentation.

Flags: needinfo?(jwatt)
Severity: normal → S3

So the DOMSVGAnimatedTransformList dtor is not called before the class's static SVGAttrTearoffTable has its dtor called.

S3 seems about right.

Flags: needinfo?(jwatt)

Given that this is a fuzzblocker and the assertion is clearly reachable, maybe it'd be worth downgrading the assertion to non-fatal for the time being?

(I assume the remain in hashtable at shutdown wording is alluding to a leak, which is not-great but also not-catastrophic.)

Flags: needinfo?(jwatt)

Oh, I guess it's potentially a bit more subtle than a leak, given the comment above the assertion. So downgrading the assertion might be a little iffy.

jkratzer, would you mind generating a pernosco trace for this?

Flags: needinfo?(jwatt) → needinfo?(jkratzer)

A pernosco session for this bug can be found here.

Flags: needinfo?(jkratzer)

dholbert does the Pernosco session provide any useful information? The fuzzers are still reporting this issue frequently.

Flags: needinfo?(dholbert)

I'm sure it does, yeah -- sorry, good to know that fuzzers are hitting this a lot. I'll take a look later on today. (Pernosco DB is rebuilding at the moment.)

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

Oh, I guess it's potentially a bit more subtle than a leak, given the comment above the assertion. So downgrading the assertion might be a little iffy.

Elaborating on this: I was worried that the comment might've been indicating that we were keeping a raw pointer to some object that had been freed but was never removed from the tearoff table.

But in fact, looking at the pernosco session, that's fortunately not what's happening. It looks like we're just reaching shutdown without having called Remove() on refcounted objects that we generated for JS to hold onto. Maybe we're taking a process-shutdown path where we don't bother to do that sort of cleanup, e.g. because the script itself happens to be the thing that's forcing us to quit, and/or just because we're aggressive about killing content processes when we don't need them?

I'm not sure whether we expect JS-owned refcounted objects to be cleaned up in that circumstance. The worry is that they might leak in some case (e.g. if we were closing this tab but the content process was kept alive due to being used by some other tab). But in that circumstance, I'm pretty sure they'd get cycle collected.

(In reply to Ting-Yu Lin [:TYLin] (UTC-8) from comment #6)

The full stack is like:

#0  0x00007f70a3d5b4a7 in mozilla::SVGAttrTearoffTable<mozilla::SVGAnimatedLength, mozilla::dom::DOMSVGAnimatedLength>::~SVGAttrTearoffTable() (this=0x7f70ad6d6960 <mozilla::sSVGAnimatedLengthTearoffTable>)
    at /home/aethanyc/Projects/gecko/dom/svg/SVGAttrTearoffTable.h:29

Notably, TYLin noticed that the first testcase here was tripping this for the DOMSVGAnimatedLength tearoff table, probably for the gradient.y1 length value in the original testcase. The new testcase (captured in pernosco) is triggering it for a different tearoff table, DOMSVGAnimatedTransformList, for a linearGradient gradientTransform attribute. But in both cases, it seems like it's just that JS is still holding onto this value when we hit shutdown time, without ever having released it.

mccr8, do you know if we're guaranteed to do GC/CC to clean up dom objects that JS is holding onto when we reach shutdown-time in cases like this, or do we sometimes just insta-quit without bothering to clean those up?

Flags: needinfo?(dholbert)

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

mccr8, do you know if we're guaranteed to do GC/CC to clean up dom objects that JS is holding onto when we reach shutdown-time in cases like this, or do we sometimes just insta-quit without bothering to clean those up?

er, meant to needinfo mccr8. :)

mccr8, see comment 17 -- I'm curious if you have insights. Specifically:

  • I'm wondering if we have any sort of GC/CC guarantee along the lines of what I alluded to (it seems we don't? which makes this assertion invalid)
  • and if we don't, is there any sort of way to detect this scenario (a flag that's set somewhere, etc), which I could add as an additional check in this assertion to rescue it from being completely invalid?

(I'm thinking we'd make the assertion more-valid by changing it from MOZ_ASSERT(!mTable) to MOZ_ASSERT(aggressivelyShuttingDownWithoutCleaningUp || !mTable) or something like that.)

Flags: needinfo?(continuation)

In builds where we care about leak checking (which are basically debug and ASan builds), we do the full shutdown sequence including GCs and CCs.

Flags: needinfo?(continuation)

Thanks. Can you recommend a way to check for that programmatically, which I could use to rescue this assertion to make it valid?

(e.g. is there a IsLeakCheckingBuild() or #define LEAK_CHECKING or something to that effect?)

(from searchfox poking, maybe NS_FREE_PERMANENT_DATA is the right thing to test for? If that's defined, I can assume that GC/CC has happened for the testcase's JS by the time we reach shutdown?)

We always care about leak checking in debug builds, and MOZ_ASSERT is only defined in debug builds, so that won't help here. (For completeness, the define is NS_FREE_PERMANENT_DATA.)

It looks like this is basically an assertion that will fail if you leak, and that kind of assertion can be problematic because you become responsible for fixing all leaks in the code. I recommend that you make it non-fatal, unless you want to spend some time investigating the leak. The JS engine has an assertion like that that I convinced them to make non-fatal. Of course, never leaking some SVG data structures is a more tractable goal than never leaking anything in JS.

The assertion assumes that GC/CC will have run before shutdown to clean up any
objects that we handed out to JS. And that assumption only holds in
leak-checked builds.

Depends on D179164

Assignee: nobody → dholbert
Status: NEW → ASSIGNED

oops, I posted a patch before seeing comment 22. :) I'll adjust the patch to just relax the assertion.

never leaking some SVG data structures is a more tractable goal than never leaking anything in JS.

In this case, the SVG structures are being held onto from the JS side, so I'll still point the finger in their direction. :)

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

In this case, the SVG structures are being held onto from the JS side, so I'll still point the finger in their direction. :)

Sure, but the JS could in turn be held alive by C++. It is hard to say what the true culprit is without digging through cycle collector logs.

Attachment #9336020 - Attachment description: Bug 1604498: Only run SVGAttrTearoffTable leak-checking assertion if we're in a leak-checked build. r?mccr8 → Bug 1604498: Downgrade a SVGAttrTearoffTable leak-checking assertion. r?mccr8

Gotcha, ok. Thanks for taking a look & for the review!

Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/419be14a6b26
Downgrade a SVGAttrTearoffTable leak-checking assertion. r=mccr8
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 115 Branch

The patch landed in nightly and beta is affected.
:dholbert, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox114 to wontfix.

For more information, please visit BugBot documentation.

Flags: needinfo?(dholbert)
Flags: needinfo?(dholbert)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: