Closed Bug 1020226 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: DOM: Workers, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox30 --- wontfix
firefox31 + verified
firefox32 + verified
firefox-esr24 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.3T --- unaffected
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: jandem, Assigned: nsm)

References

Details

(Keywords: crash, regression)

Attachments

(6 files, 1 obsolete file)

Attached file 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.
Flags: needinfo?(nsm.nikhil)
Attachment #8434043 - Attachment mime type: application/force-download → application/pdf
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.
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
Flags: in-testsuite?
Keywords: crash, regression
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...
I don't think mozregression supports bisection on fx-team, so I'll run some Try builds to attempt to confirm.
Definitely started on the pdf.js update. Will work on bisecting over that range next.
Blocks: 1007627
This can cause crashes when opening certain PDFs, so we should track this for 32.
STR:
1. Open attachment 8434200 [details]
2. Refresh the opened page
(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
Managed to reproduce. Will look at this.
Flags: needinfo?(nsm.nikhil)
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?
Attachment #8434776 - Flags: review?(khuey)
Attachment #8434776 - Flags: review?(bent.mozilla)
Assignee: nobody → nsm.nikhil
Comment on attachment 8434776 [details] [diff] [review]
Fix unhandled Promise rejection handling in workers.

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

Nice.
Attachment #8434776 - Flags: review?(khuey)
Attachment #8434776 - Flags: review?(bent.mozilla)
Attachment #8434776 - Flags: review+
Comment on attachment 8434776 [details] [diff] [review]
Fix unhandled Promise rejection handling in workers.

Please include a test too?
(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 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.
Attachment #8435124 - Flags: review?(khuey) → review-
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?
https://hg.mozilla.org/mozilla-central/rev/fc7aa8ef3540
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Tracking to make sure it gets into 31
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 :)
Status: RESOLVED → VERIFIED
Flags: in-testsuite? → in-testsuite+
(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).
Flags: needinfo?(nsm.nikhil)
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
Attachment #8438040 - Flags: review+
Attachment #8438040 - Flags: approval-mozilla-beta?
Attachment #8438040 - Flags: approval-mozilla-b2g30?
Flags: needinfo?(nsm.nikhil)
Attachment #8438040 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
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.
Flags: needinfo?(nsm.nikhil)
The root problem is in Gecko 30, but it was fixed too late in the cycle to make Fx30.
Flags: needinfo?(nsm.nikhil)
Attachment #8438040 - Flags: approval-mozilla-b2g30? → approval-mozilla-b2g30+
You need to log in before you can comment on or make changes to this bug.