1665299 - Crash in [@ nsWebBrowserPersist::EndDownload]

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Bug 1663499 added the assertion triggered here.

In this case, the user called a function from the add-on Media Helper 1.16.0 (id: {85603edc-044f-4391-9a95-a94682f449b4}) to download an image from instagram.

Crash report: https://crash-stats.mozilla.org/report/index/094ad849-839f-4cb1-9f04-801180200916

Top 10 frames of crashing thread:

0 xul.dll nsWebBrowserPersist::EndDownload dom/webbrowserpersist/nsWebBrowserPersist.cpp:2317
1 xul.dll mozilla::detail::RunnableMethodImpl< xpcom/threads/nsThreadUtils.h:1240
2 xul.dll mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal xpcom/threads/TaskController.cpp:514
3 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1234
4 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:87
5 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:327
6 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:309
7 xul.dll nsBaseAppShell::Run widget/nsBaseAppShell.cpp:137
8 xul.dll nsAppShell::Run widget/windows/nsAppShell.cpp:602
9 xul.dll nsAppStartup::Run toolkit/components/startup/nsAppStartup.cpp:270

Comment 1

[:Gijs (he/him)]

I tried for a bit with the add-on but can't reproduce a crash...

Really what I want is on nightly / debug builds, to capture the stack of the first invocation, and store that as metadata for the second invocation. Right now all I know is the second invocation is coming from a runnablemethod call, so it's probably the one from https://hg.mozilla.org/mozilla-central/file/2cca9cb89b46d67e1e25051495cb89f71b856c32/dom/webbrowserpersist/nsWebBrowserPersist.cpp#l1080 - but even there optimization muddies the water. Anyway - without knowing what the first invocation is, it's hard to know how to fix the "double end download" - apart from just wallpapering over it. Gabriele, is there a way of capturing the stack and not crashing, but then submitting the stack when we hit the second EndDownload?

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1663499

Comment 3

[Gabriele Svelto [:gsvelto]]

Not right now. It's theoretically possible however. It would involve capturing a minidump of the first call and then attaching it to the second one as a paired minidump. It would require some surgery to the crash reporting code but it should be possible.

Comment 4

[:Gijs (he/him)]

Attachment: Bug 1665299 - no-op when cancelling causes us to call EndDownload more than once in webbrowserpersist to avoid crashes, r?smaug

Comment 5

[:Gijs (he/him)]

Gonna leave-open this to see if the patch helps before closing.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15747596bf1e
no-op when cancelling causes us to call EndDownload more than once in webbrowserpersist to avoid crashes, r=smaug

Comment 7

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/15747596bf1e

Comment 8

[Julien Cristau [:jcristau]]

Looks like we're still seeing these, e.g. bp-8cedd7d1-5a6e-4629-b5ac-7e1d30200927

Comment 9

[:Gijs (he/him)]

Attachment: Bug 1665299 - remove diagnostic assert for failing too often without reproducible steps, r?smaug

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b739dc65059c
remove diagnostic assert for failing too often without reproducible steps, r=smaug

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/b739dc65059c

Comment 12

[:Gijs (he/him)]

Attachment: beta-assert-downloads.txt

(Beta patch because it required rebasing)

Approval Request Comment
[Feature/Bug causing the regression]: bug 1663499
[User impact if declined]: crashes on devedition
[Is this code covered by automated tests?]: yes, but not the crashes (wouldn't that have been nice...)
[Has the fix been verified in Nightly?]: no, but it's a pretty straightforward change from DIAGNOSTIC_ASSERT to ASSERT + early return
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: see above - the patch is really small and we're just making the behaviour more reasonable and less crashy.
[String changes made/needed]: nope

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 9178314 [details] [diff] [review]
beta-assert-downloads.txt

approved for 82.0b5

Comment 14

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0f3b9077d2cb