Closed Bug 1534172 Opened 6 years ago Closed 5 years ago

Convert RemoteController.js back into a jsm

Categories

(Toolkit :: XUL Widgets, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox67 --- affected

People

(Reporter: Gijs, Unassigned)

References

Details

(Whiteboard: [fxperf:p3])

The way bug 1395795 was fixed means we now re-load and re-evaluate RemoteController.js for every browser in a window. In my current window, that means we've loaded and evaluated it 101 times. That's not very nice. We should come up with a better way.

Ehsan, can you expand on what bug 1395795 was trying to accomplish? From the scant comments on the bug, I don't really understand what wrappers it's talking about, why that generates jank, and if there are alternatives that don't have the downsides the current solution does.

(In reply to :Gijs (he/him) from comment #0)

Ehsan, can you expand on what bug 1395795 was trying to accomplish? From the scant comments on the bug, I don't really understand what wrappers it's talking about, why that generates jank, and if there are alternatives that don't have the downsides the current solution does.

Actually needinfo'ing this time.

Flags: needinfo?(ehsan)
Whiteboard: [fxperf] → [fxperf:p3]

Sure, see this profile https://perfht.ml/2CfRCXh from bug 1395274.

The cost being discussed here was the cost of wrapping the arguments passed to enableDisableCommands when wrapping them inside a CCW as you can see in that profile. By moving the code for that function outside of a JSM, we made sure the chrome code on the caller and callee frames run in the same compartment and therefore no CCW would be necessary for the call.

I believe that the compartment setup for chrome code right now has changed drastically since that time, though I haven't followed all of the details, so putting this code back in a JSM without incurring this regression might be viable now...

Flags: needinfo?(ehsan)

(In reply to :Ehsan Akhgari from comment #2)

I believe that the compartment setup for chrome code right now has changed drastically since that time, though I haven't followed all of the details, so putting this code back in a JSM without incurring this regression might be viable now...

Who would know whether that would make sense?

The other option here seems to be to make the thing more uniquely named and load it once per chrome window instead of once per browser, which would also mostly fix this issue.

Flags: needinfo?(ehsan)
Priority: -- → P3

(In reply to :Gijs (he/him) from comment #3)

(In reply to :Ehsan Akhgari from comment #2)

I believe that the compartment setup for chrome code right now has changed drastically since that time, though I haven't followed all of the details, so putting this code back in a JSM without incurring this regression might be viable now...

Who would know whether that would make sense?

I believe this is now fixed given that bug 1512029 and bug 1514210 are both fixed, but jandem would know best if we can still have cases where chrome code can run in more than one system principal compartment?

Flags: needinfo?(ehsan) → needinfo?(jdemooij)

(In reply to :Ehsan Akhgari from comment #4)

I believe this is now fixed given that bug 1512029 and bug 1514210 are both fixed, but jandem would know best if we can still have cases where chrome code can run in more than one system principal compartment?

Yes chrome code running in windows, sandboxes, JSMs should all be same-compartment now so I think moving it back into a JSM will be fine :)

Flags: needinfo?(jdemooij)
Summary: Avoid evaluating RemoteController.js once for every browser we create → Convert RemoteController.js back into a jsm

As far as I see, this was fixed in bug 1604003.

Please reopen if there is still more to do here.
Bug 1558520 will change more of this stuff.

Status: NEW → RESOLVED
Closed: 5 years ago
Depends on: 1604003
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.