1020226 - Still seeing null-dereference crashes when a rejected Promise survives to worker shutdown

Status: VERIFIED FIXED

(Core :: DOM: Workers, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: PDF

This looks identical to bug 967005, but it was fixed a while ago.

With a recent Nightly + a clean profile, if I open the attached pdf file I get the following crash:

https://crash-stats.mozilla.com/report/index/276f2315-f4ea-4951-9cc5-446932140604

You may have to try it a few times, but it usually crashes a few seconds after opening it for the first time.

Comment 1

[Jan de Mooij [:jandem]]

You have to make sure Firefox's pdf.js opens the PDF file, it shouldn't offer a "Download file" dialog. I just verified it crashes when I create a new profile and open the attachment on OS X.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Last good revision: cf89b5d018f8 (2014-05-09)
First bad revision: 7586c29906b5 (2014-05-10)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cf89b5d018f8&tochange=7586c29906b5

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Last good revision: 03d54c37d264
First bad revision: 11291a7123f4
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=03d54c37d264&tochange=11291a7123f4

Conveniently, there's a pdf.js update in there...

Comment 4

[Ryan VanderMeulen [:RyanVM]]

I don't think mozregression supports bisection on fx-team, so I'll run some Try builds to attempt to confirm.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Lots of Promise-related changes there too:
https://bugzilla.mozilla.org/show_bug.cgi?id=1007627#c0

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Definitely started on the pdf.js update. Will work on bisecting over that range next.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

We have a winner:
https://github.com/mozilla/pdf.js/commit/caad87eba5bfb05a22d0b745d86f2654ce36fa0b

Comment 8

[Jan de Mooij [:jandem]]

This can cause crashes when opening certain PDFs, so we should track this for 32.

Comment 9

[Yury Delendik (:yury)]

Attachment: worker with rejected promise

Comment 10

[Yury Delendik (:yury)]

Attachment: Minimal test case to replicate the issue

Comment 11

[Yury Delendik (:yury)]

STR:
1. Open attachment 8434200 [details]
2. Refresh the opened page

Comment 12

[Yury Delendik (:yury)]

(In reply to Jan de Mooij [:jandem] from comment #8)
> This can cause crashes when opening certain PDFs, so we should track this
> for 32.

I was able to replicate that on FF29 using the minimal test case

Comment 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Managed to reproduce. Will look at this.

Comment 14

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Fix unhandled Promise rejection handling in workers.

When the script prevents the promise from actually being rejected until the worker is Canceled, the AddFeature() call fails and MaybeReportReject() is called, which is correct. But the Promise is never going to be notified of the worker going away, so it should report the rejection and then prevent it from being called again in the destructor when GCed.

Kyle, Ben original reviewed this in Bug 967005, but he is on PTO, so if you have time, could you review it?

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8434776 [details] [diff] [review]
Fix unhandled Promise rejection handling in workers.

Review of attachment 8434776 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8434776 [details] [diff] [review]
Fix unhandled Promise rejection handling in workers.

Please include a test too?

Comment 17

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16)
> Comment on attachment 8434776 [details] [diff] [review]
> Fix unhandled Promise rejection handling in workers.
> 
> Please include a test too?

I'm not sure how to write a automated test for this. Can I test reloading a page via mochitest?

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

With a subframe, yes.

Comment 19

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Test case to crash browser when using Promises on workers.

Test case.

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8435124 [details] [diff] [review]
Test case to crash browser when using Promises on workers.

Review of attachment 8435124 [details] [diff] [review]:
-----------------------------------------------------------------

The worker should post a message up to the window telling it that it has started up and created the Promise, rather than doing a setTimeout(1000) to wait for it.

Comment 21

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Test case to crash browser when using Promises on workers.

How about now?

Comment 22

[Ryan VanderMeulen [:RyanVM]]

So it sounds like this is actually a regression from bug 967005 (exposed by later pdf.js changes). In that case, we'll probably need to land this on Fx31 as well?

Comment 23

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

folded
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fc7aa8ef3540

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Tracking to make sure it gets into 31

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Confirmed that the PDF testcase no longer crashes :)

Nikhil, please request Aurora and b2g30 approval on this patch as soon as you get a chance :)

Comment 27

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #26)
> Nikhil, please request Aurora and b2g30 approval on this patch as soon as
> you get a chance :)

Setting needinfo? from Nikhil - I believe it's now Aurora (31) then is now Beta (31).

Comment 28

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Folded fix+testcase

Carrying forward r+ from khuey, this is the folded patch as landed in m-c and the one that should end up on other trees

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 967005
User impact if declined: Using Promises on web workers can lead to a crash.
Testing completed (on m-c, etc.): 
Yes, automated test case included and PDF.js crashes are fixed as confirmed above.

Risk to taking this patch (and alternatives if risky): 
None

String or IDL/UUID changes made by this patch:
None

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/055bf6fd1658

Comment 30

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 31.02
Build Id:20140616143923
OS: Windows 7 x64, Ubuntu 12.10 x32, Mac Os X 10.8.5

Comment 31

[Chris Peterson [:cpeterson]]

Nikhil: is this Promise crash fix relevant for B2G 1.4? This bug's status flags say b2g1.4 is affected, but firefox30 is wontfix.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

The root problem is in Gecko 30, but it was fixed too late in the cycle to make Fx30.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/c0511ca22db6