1440941 - Aborting fetch with a lot of requests causes browser crash (@mozilla::dom::FetchDriver::OnStartRequest)

Status: VERIFIED FIXED

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

Description

[Sören Hentzschel]

Attachment: bookmarks_organizer-2.0.0.xpi

I am working on a new version of my WebExtension Bookmarks Organizer. One change is that I want to abort the request after x seconds.

There are no problems with the old code which doesn't abort:

const response = await fetch(bookmark.url, {
  cache : 'no-store',
  credentials : 'include',
  method : method
});

But once I change the code to the following:

const controller = new AbortController();
const signal = controller.signal;

setTimeout(() => controller.abort(), 3000);

const response = await fetch(bookmark.url, {
  cache : 'no-store',
  credentials : 'include',
  method : method,
  signal : signal
});

… Firefox crashes on my profile with more than 5,000 bookmarks after a few seconds. On a new profile with only 9 bookmarks there is no crash. It happens in every version from Firefox Stable 58 (tested via about:debugging) to Firefox 60 Nightly.

Here is one of my crash reports:
https://crash-stats.mozilla.com/report/index/d9227073-3d78-4205-85f3-2b6570180224

(@ mozilla::dom::FetchDriver::OnStartRequest)

ni? :baku because you implemented aborting fetch requests.

Do you have any idea what's going on there? Is there anything what I can do to help finding the cause? Do you need a backup of my bookmarks? (the crash is reproducible on my system when exporting the bookmarks and importing in a new profile).

I attached a preview version of the new add-on version. You can open the add-on interface via the toolbar button. Pressing the check bookmarks button on the bottom with the default option in the dropdown on the bottom left causes the crash after a few seconds for me.

Comment 1

[Sören Hentzschel]

Here is a report from a Windows user:
https://crash-stats.mozilla.com/report/index/95a92eb7-e654-45e4-b53b-e53a40180224

Comment 2

[Andrea Marchesini [:baku]]

Attachment: fetch_crash.patch

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bfc4dd20e01
FetchDriver should check if the operation has been already aborted when OnStartRequest is called, r=bkelly

Comment 4

[Sören Hentzschel]

Thank you for the very quick patch! I will test once it's in Firefox Nightly. If it works and won't be backed out for whatever reason, is there a chance that this bugfix will be uplifted to Firefox 59 Beta or is it too risky because it's already so late in the beta cycle?

Comment 5

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/1bfc4dd20e01

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Andrea, can you uplift to beta since its a pretty safe fix?

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8954123 [details] [diff] [review]
fetch_crash.patch

Approval Request Comment
[Feature/Bug causing the regression]: Abort API
[User impact if declined]: a crash
[Is this code covered by automated tests?]: race condition
[Has the fix been verified in Nightly?]: n/a
[Needs manual test from QE? If yes, steps to reproduce]: There is an STR.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: When OnStartRequest is called, this patch introduces a new check about mChannel being already nullified.
[String changes made/needed]: none

Comment 8

[Sören Hentzschel]

I can verify the fix works for the STR in comment #0.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8954123 [details] [diff] [review]
fetch_crash.patch

Minor API fix, verified in nightly, looks fine for uplift for beta 14.

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f83587c150ab

Comment 11

[Andrei Vaida [:avaida]]

Sören, could you please confirm whether this is fixed on Firefox 59 Beta 14 as well?

You can find the build here: https://archive.mozilla.org/pub/firefox/candidates/59.0b14-candidates/build1/.

Comment 12

[Sören Hentzschel]

I can confirm it's fixed in Firefox 59 Beta 14.