[Shutdown] Avoid js::NukeCrossCompartmentWrappers in SDK on shutdown

RESOLVED FIXED in 1.14

Status

defect
P1
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: BenWa, Assigned: mossop)

Tracking

(Blocks 1 bug)

unspecified
1.14
x86
macOS
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy:p1][sps])

Attachments

(1 attachment)

Reporter

Description

7 years ago
During shutdown the addon SDK calls 'js::NukeCrossCompartmentWrappers' from 'unloadSandbox()@bootsrap.js'.

This is takes about 141ms on my machine on shutdown. In bug 818296 we're skipping it completely for window destruction on optimized shutdown only. We should consider doing the same in the SDK.
Assignee

Updated

7 years ago
Priority: -- → P1
Reporter

Comment 1

7 years ago
I've received more shutdown reports from users indicating that this is a problem. So far the highest I've seen is 500ms.
Reporter

Comment 2

7 years ago
Any chance someone can look at this? It's easy to code, will give a sizeable improvement to your shutdown telemetry has confirmed by the performance reporter (several slow shutdown profiles show 100-500ms spend in this function for add-on heavy profiles).

This will take a long time for add-on to be repack with this change so the sooner we get it in the better.
On the profile in

http://people.mozilla.com/~bgirard/cleopatra/?report=ea216713a20097f657da61b0aad4452b58963226#report=ea216713a20097f657da61b0aad4452b58963226&search=nuke

It looks like we get to nukeModules earlier than we want to ever put the call to exit(0). If this is something that should be safe to skip, probably the safe way to do it is to first move it to a xpcom-shutdown-* message. That way we will at some point move write poisoning and exit(0) past it.

Also note that this is from a timer. If it is at all important to run during shutdown, that should be avoided. As we shutdown faster we will at some point exit before the time set for the timer.

Updated

6 years ago
Whiteboard: [Snappy][sps] → [Snappy:p1][sps]
Matteo, can you take a look at this please?
Assignee: nobody → zer0
Actually never mind I can see the fix is straightforward so I'll just take it.
Assignee: zer0 → dtownsend+bugmail
Assignee

Updated

6 years ago
Attachment #716165 - Flags: review?(poirot.alex)
Attachment #716165 - Flags: review?(poirot.alex) → review+

Comment 8

6 years ago
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/4e1166445ccad9d1f0060387b481a32b52012cf9
Bug 819454: Avoid nuking module sandboxes when the application is shutting down.

https://github.com/mozilla/addon-sdk/commit/03f0890b41d10c2633262a095be09a21238cd643
Merge pull request #809 from Mossop/bug819454

Bug 819454: Avoid nuking module sandboxes when the application is shutting down. r=ochemeau
Would like to spin this into 1.14 once it's been through tests please Wes.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.14

Comment 10

6 years ago
Commit pushed to stabilization at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/f496376919260068e21addc05e962ab49ee40cb8
Merge pull request #809 from Mossop/bug819454

Bug 819454: Avoid nuking module sandboxes when the application is shutting down. r=ochemeau(cherry picked from commit 03f0890b41d10c2633262a095be09a21238cd643)
Assignee

Updated

6 years ago
Blocks: 843638
Reporter

Comment 11

6 years ago
For the platform we only disable this if we doing a non debug shutdown. I don't know enough about NukeCrossCompartmentWrappers to advise here.

Kyle can you take a look at the patch that landed here?
(In reply to Benoit Girard (:BenWa) from comment #11)
> For the platform we only disable this if we doing a non debug shutdown. I
> don't know enough about NukeCrossCompartmentWrappers to advise here.
> 
> Kyle can you take a look at the patch that landed here?

I'm not sure what input you want from me here ...
You need to log in before you can comment on or make changes to this bug.