Skip to content

Commit d6fb2b8

Browse files
rgarciargarcia
committed
Separate Chromium configure execution paths
1 parent 91ea648 commit d6fb2b8

3 files changed

Lines changed: 152 additions & 110 deletions

File tree

‎server/cmd/api/api/chromium_configure.go‎

Lines changed: 111 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ func (st *chromiumConfigureState) cleanup() {
5050
}
5151

5252
// ChromiumConfigure batched Chromium/session configuration plus optional navigation.
53-
func (s *ApiService) ChromiumConfigure(ctx context.Context, request oapi.ChromiumConfigureRequestObject) (resp oapi.ChromiumConfigureResponseObject, err error) {
53+
func (s *ApiService) ChromiumConfigure(ctx context.Context, request oapi.ChromiumConfigureRequestObject) (oapi.ChromiumConfigureResponseObject, error) {
5454
start := time.Now()
5555

5656
if request.Body == nil {
@@ -80,7 +80,60 @@ func (s *ApiService) ChromiumConfigure(ctx context.Context, request oapi.Chromiu
8080
s.chromiumConfigMu.Lock()
8181
defer s.chromiumConfigMu.Unlock()
8282

83-
needsStop := chromiumNeedsStopCycle(st)
83+
var configureResp oapi.ChromiumConfigureResponseObject
84+
switch chromiumConfigureModeFor(st) {
85+
case chromiumConfigureModeLive:
86+
configureResp = s.chromiumConfigureLive(ctx, st)
87+
case chromiumConfigureModeRestart:
88+
configureResp = s.chromiumConfigureRestart(ctx, st, spec)
89+
}
90+
if configureResp != nil {
91+
return configureResp, nil
92+
}
93+
94+
if spec.needsNav {
95+
if err := chromiumDoNavigate(ctx, s, spec); err != nil {
96+
logger.FromContext(ctx).Warn("start_url dispatch failed", "error", err)
97+
}
98+
}
99+
100+
logger.FromContext(ctx).Info("chromium configure finished", "elapsed", time.Since(start).String())
101+
return oapi.ChromiumConfigure200JSONResponse{Ok: true}, nil
102+
}
103+
104+
type chromiumConfigureMode uint8
105+
106+
const (
107+
chromiumConfigureModeLive chromiumConfigureMode = iota
108+
chromiumConfigureModeRestart
109+
)
110+
111+
func chromiumConfigureModeFor(st *chromiumConfigureState) chromiumConfigureMode {
112+
if st.hasProfile ||
113+
len(st.extItems) > 0 ||
114+
policiesContentNonEmpty(st.chromePoliciesJSON) ||
115+
flagsContentNonEmpty(st.chromiumFlagsJSON) {
116+
return chromiumConfigureModeRestart
117+
}
118+
return chromiumConfigureModeLive
119+
}
120+
121+
func (s *ApiService) chromiumConfigureLive(ctx context.Context, st *chromiumConfigureState) oapi.ChromiumConfigureResponseObject {
122+
if st.displayJSON == nil || strings.TrimSpace(*st.displayJSON) == "" {
123+
return nil
124+
}
125+
126+
displayPlan, displayResp := chromiumPrepareDisplay(ctx, s, st.displayJSON)
127+
if displayResp != nil {
128+
return displayResp
129+
}
130+
if displayPlan == nil {
131+
return nil
132+
}
133+
return chromiumRunPatchDisplay(ctx, s, displayPlan.body)
134+
}
135+
136+
func (s *ApiService) chromiumConfigureRestart(ctx context.Context, st *chromiumConfigureState, spec startURLParsed) (resp oapi.ChromiumConfigureResponseObject) {
84137
chromiumStopped := false
85138
restartAfterStop := func() error {
86139
if !chromiumStopped {
@@ -99,103 +152,80 @@ func (s *ApiService) ChromiumConfigure(ctx context.Context, request oapi.Chromiu
99152
return
100153
}
101154
resp = cfg500ConfigureStep(chromiumConfigureStepStart, restartErr.Error())
102-
err = nil
103155
}
104156
}()
105157

106-
if needsStop {
107-
logger.FromContext(ctx).Info("chromium configure (stop/start path)")
108-
if err := s.stopChromium(ctx); err != nil {
109-
return cfg500ConfigureStep(chromiumConfigureStepStop, err.Error()), nil
110-
}
111-
chromiumStopped = true
112-
113-
policyOverrides, err := chromiumValidatePolicies(st.chromePoliciesJSON)
114-
if err != nil {
115-
return cfgResponseFromStepError(chromiumConfigureStepPolicies, err), nil
116-
}
117-
if err := chromiumApplyPolicies(ctx, s, policyOverrides); err != nil {
118-
return cfgResponseFromStepError(chromiumConfigureStepPolicies, err), nil
119-
}
158+
logger.FromContext(ctx).Info("chromium configure (stop/start path)")
159+
if err := s.stopChromium(ctx); err != nil {
160+
return cfg500ConfigureStep(chromiumConfigureStepStop, err.Error())
161+
}
162+
chromiumStopped = true
120163

121-
if reqMsgs, ierr := chromiumApplyExtensions(ctx, s, st.extItems); reqMsgs != "" {
122-
return cfg400(fmt.Sprintf("%s: %s", chromiumConfigureStepExtensions, reqMsgs)), nil
123-
} else if ierr != nil {
124-
return cfg500ConfigureStep(chromiumConfigureStepExtensions, ierr.Error()), nil
125-
}
164+
policyOverrides, err := chromiumValidatePolicies(st.chromePoliciesJSON)
165+
if err != nil {
166+
return cfgResponseFromStepError(chromiumConfigureStepPolicies, err)
167+
}
168+
if err := chromiumApplyPolicies(ctx, s, policyOverrides); err != nil {
169+
return cfgResponseFromStepError(chromiumConfigureStepPolicies, err)
170+
}
126171

127-
if st.displayJSON != nil && strings.TrimSpace(*st.displayJSON) != "" {
128-
displayPlan, displayResp := chromiumPrepareDisplay(ctx, s, st.displayJSON)
129-
if displayResp != nil {
130-
return displayResp, nil
131-
}
132-
if displayPlan != nil {
133-
stopped, stopErr := s.stopActiveRecordings(ctx)
134-
if stopErr != nil {
135-
return cfg500ConfigureStep(chromiumConfigureStepDisplay, fmt.Sprintf("failed to stop recordings: %v", stopErr)), nil
136-
}
137-
if len(stopped) > 0 {
138-
defer func() {
139-
go s.startNewRecordingSegments(context.WithoutCancel(ctx), stopped)
140-
}()
141-
}
142-
if rr := chromiumDisplayApplyWhileStopped(ctx, s, displayPlan); rr != nil {
143-
return rr, nil
144-
}
145-
}
146-
}
172+
if reqMsgs, ierr := chromiumApplyExtensions(ctx, s, st.extItems); reqMsgs != "" {
173+
return cfg400(fmt.Sprintf("%s: %s", chromiumConfigureStepExtensions, reqMsgs))
174+
} else if ierr != nil {
175+
return cfg500ConfigureStep(chromiumConfigureStepExtensions, ierr.Error())
176+
}
147177

148-
flagsPlan, err := chromiumValidateFlags(st.chromiumFlagsJSON)
149-
if err != nil {
150-
return cfgResponseFromStepError(chromiumConfigureStepFlags, err), nil
151-
}
152-
if err := chromiumMergeFlags(ctx, s, flagsPlan); err != nil {
153-
return cfgResponseFromStepError(chromiumConfigureStepFlags, err), nil
178+
if st.displayJSON != nil && strings.TrimSpace(*st.displayJSON) != "" {
179+
displayPlan, displayResp := chromiumPrepareDisplay(ctx, s, st.displayJSON)
180+
if displayResp != nil {
181+
return displayResp
154182
}
155-
156-
if st.hasProfile {
157-
preparedProfile, cleanupProfile, err := chromiumPrepareProfileArchive(st.profileTemp, st.stripComponents)
158-
if cleanupProfile != nil {
159-
defer cleanupProfile()
160-
}
161-
if err != nil {
162-
return cfg500ConfigureStep(chromiumConfigureStepProfile, err.Error()), nil
183+
if displayPlan != nil {
184+
stopped, stopErr := s.stopActiveRecordings(ctx)
185+
if stopErr != nil {
186+
return cfg500ConfigureStep(chromiumConfigureStepDisplay, fmt.Sprintf("failed to stop recordings: %v", stopErr))
163187
}
164-
if spec.needsNav {
165-
if err := stripProfileSessionRestore(preparedProfile); err != nil {
166-
return cfg500ConfigureStep(chromiumConfigureStepProfile, err.Error()), nil
167-
}
188+
if len(stopped) > 0 {
189+
defer func() {
190+
go s.startNewRecordingSegments(context.WithoutCancel(ctx), stopped)
191+
}()
168192
}
169-
if err := chromiumInstallPreparedProfile(preparedProfile); err != nil {
170-
return cfg500ConfigureStep(chromiumConfigureStepProfile, err.Error()), nil
193+
if rr := chromiumDisplayApplyWhileStopped(ctx, s, displayPlan); rr != nil {
194+
return rr
171195
}
172196
}
197+
}
198+
199+
flagsPlan, err := chromiumValidateFlags(st.chromiumFlagsJSON)
200+
if err != nil {
201+
return cfgResponseFromStepError(chromiumConfigureStepFlags, err)
202+
}
203+
if err := chromiumMergeFlags(ctx, s, flagsPlan); err != nil {
204+
return cfgResponseFromStepError(chromiumConfigureStepFlags, err)
205+
}
173206

174-
if err := restartAfterStop(); err != nil {
175-
return cfg500ConfigureStep(chromiumConfigureStepStart, err.Error()), nil
207+
if st.hasProfile {
208+
preparedProfile, cleanupProfile, err := chromiumPrepareProfileArchive(st.profileTemp, st.stripComponents)
209+
if cleanupProfile != nil {
210+
defer cleanupProfile()
176211
}
177-
} else {
178-
if st.displayJSON != nil && strings.TrimSpace(*st.displayJSON) != "" {
179-
displayPlan, displayResp := chromiumPrepareDisplay(ctx, s, st.displayJSON)
180-
if displayResp != nil {
181-
return displayResp, nil
182-
}
183-
if displayPlan != nil {
184-
if rr := chromiumRunPatchDisplay(ctx, s, displayPlan.body); rr != nil {
185-
return rr, nil
186-
}
212+
if err != nil {
213+
return cfg500ConfigureStep(chromiumConfigureStepProfile, err.Error())
214+
}
215+
if spec.needsNav {
216+
if err := stripProfileSessionRestore(preparedProfile); err != nil {
217+
return cfg500ConfigureStep(chromiumConfigureStepProfile, err.Error())
187218
}
188219
}
189-
}
190-
191-
if spec.needsNav {
192-
if err := chromiumDoNavigate(ctx, s, spec); err != nil {
193-
logger.FromContext(ctx).Warn("start_url dispatch failed", "error", err)
220+
if err := chromiumInstallPreparedProfile(preparedProfile); err != nil {
221+
return cfg500ConfigureStep(chromiumConfigureStepProfile, err.Error())
194222
}
195223
}
196224

197-
logger.FromContext(ctx).Info("chromium configure finished", "elapsed", time.Since(start).String())
198-
return oapi.ChromiumConfigure200JSONResponse{Ok: true}, nil
225+
if err := restartAfterStop(); err != nil {
226+
return cfg500ConfigureStep(chromiumConfigureStepStart, err.Error())
227+
}
228+
return nil
199229
}
200230

201231
type startURLParsed struct {
@@ -300,13 +330,6 @@ type chromiumDisplayPlan struct {
300330
refreshRate int
301331
}
302332

303-
func chromiumNeedsStopCycle(st *chromiumConfigureState) bool {
304-
return st.hasProfile ||
305-
len(st.extItems) > 0 ||
306-
policiesContentNonEmpty(st.chromePoliciesJSON) ||
307-
flagsContentNonEmpty(st.chromiumFlagsJSON)
308-
}
309-
310333
func policiesContentNonEmpty(s *string) bool {
311334
if !policiesNonEmpty(s) {
312335
return false

‎server/cmd/api/api/chromium_configure_test.go‎

Lines changed: 25 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -29,30 +29,33 @@ func TestPoliciesContentNonEmpty(t *testing.T) {
2929
require.True(t, policiesContentNonEmpty(&real))
3030
}
3131

32-
func TestChromiumConfigureActionableFlags(t *testing.T) {
33-
emptyFlags := `{"flags":[]}`
34-
realFlags := `{"flags":["--kiosk"]}`
32+
func TestChromiumConfigureModeFor(t *testing.T) {
33+
stringPtr := func(value string) *string { return &value }
3534

36-
st := &chromiumConfigureState{chromiumFlagsJSON: &emptyFlags}
37-
require.Equal(t, 0, cfgActionables(st))
38-
require.False(t, chromiumNeedsStopCycle(st))
39-
40-
st = &chromiumConfigureState{chromiumFlagsJSON: &realFlags}
41-
require.Equal(t, 1, cfgActionables(st))
42-
require.True(t, chromiumNeedsStopCycle(st))
43-
}
44-
45-
func TestChromiumConfigureActionablePolicies(t *testing.T) {
46-
emptyPolicies := `{}`
47-
realPolicies := `{"QuicAllowed":false}`
48-
49-
st := &chromiumConfigureState{chromePoliciesJSON: &emptyPolicies}
50-
require.Equal(t, 0, cfgActionables(st))
51-
require.False(t, chromiumNeedsStopCycle(st))
35+
tests := []struct {
36+
name string
37+
state chromiumConfigureState
38+
want chromiumConfigureMode
39+
}{
40+
{name: "no restart fields", want: chromiumConfigureModeLive},
41+
{name: "display only", state: chromiumConfigureState{displayJSON: stringPtr(`{"width":1280}`)}, want: chromiumConfigureModeLive},
42+
{name: "start URL only", state: chromiumConfigureState{startURLRaw: stringPtr("https://example.com")}, want: chromiumConfigureModeLive},
43+
{name: "empty policies", state: chromiumConfigureState{chromePoliciesJSON: stringPtr(`{}`)}, want: chromiumConfigureModeLive},
44+
{name: "nonempty policies", state: chromiumConfigureState{chromePoliciesJSON: stringPtr(`{"QuicAllowed":false}`)}, want: chromiumConfigureModeRestart},
45+
{name: "invalid policies", state: chromiumConfigureState{chromePoliciesJSON: stringPtr(`{bad-json`)}, want: chromiumConfigureModeRestart},
46+
{name: "empty flags", state: chromiumConfigureState{chromiumFlagsJSON: stringPtr(`{"flags":[]}`)}, want: chromiumConfigureModeLive},
47+
{name: "nonempty flags", state: chromiumConfigureState{chromiumFlagsJSON: stringPtr(`{"flags":["--kiosk"]}`)}, want: chromiumConfigureModeRestart},
48+
{name: "invalid flags", state: chromiumConfigureState{chromiumFlagsJSON: stringPtr(`{bad-json`)}, want: chromiumConfigureModeRestart},
49+
{name: "profile", state: chromiumConfigureState{hasProfile: true}, want: chromiumConfigureModeRestart},
50+
{name: "extensions", state: chromiumConfigureState{extItems: []extensionZipItem{{name: "test"}}}, want: chromiumConfigureModeRestart},
51+
{name: "display and extension", state: chromiumConfigureState{displayJSON: stringPtr(`{"width":1280}`), extItems: []extensionZipItem{{name: "test"}}}, want: chromiumConfigureModeRestart},
52+
}
5253

53-
st = &chromiumConfigureState{chromePoliciesJSON: &realPolicies}
54-
require.Equal(t, 1, cfgActionables(st))
55-
require.True(t, chromiumNeedsStopCycle(st))
54+
for _, tt := range tests {
55+
t.Run(tt.name, func(t *testing.T) {
56+
require.Equal(t, tt.want, chromiumConfigureModeFor(&tt.state))
57+
})
58+
}
5659
}
5760

5861
func TestChromiumStartURLSpec(t *testing.T) {

‎server/e2e/e2e_chromium_configure_powerset_test.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ func TestChromiumConfigureMultipartPowerset(t *testing.T) {
4444

4545
matrix := []int{
4646
matDisplay,
47+
matStartURL,
4748
matPolicy | matKioskFlags,
4849
matExtension,
4950
matDisplay | matPolicy | matKioskFlags | matExtension | matStartURL,
@@ -73,6 +74,9 @@ func TestChromiumConfigureMultipartPowerset(t *testing.T) {
7374
defer func() { _ = c.Stop(context.WithoutCancel(ctx)) }()
7475

7576
require.NoError(t, c.WaitReady(ctx))
77+
require.NoError(t, c.WaitDevTools(ctx))
78+
browserWebSocketBefore, err := fetchBrowserWebSocketURL(ctx, c)
79+
require.NoError(t, err)
7680

7781
var body bytes.Buffer
7882
w := multipart.NewWriter(&body)
@@ -89,10 +93,22 @@ func TestChromiumConfigureMultipartPowerset(t *testing.T) {
8993
"bits=%02x unexpected status=%s body=%s", bits, rsp.Status(), string(rsp.Body))
9094
require.NotNil(t, rsp.JSON200, "want ok JSON")
9195
require.True(t, rsp.JSON200.Ok)
96+
97+
browserWebSocketAfter, err := fetchBrowserWebSocketURL(ctx, c)
98+
require.NoError(t, err)
99+
if chromiumConfigurePowersetRestarts(bits) {
100+
require.NotEqual(t, browserWebSocketBefore, browserWebSocketAfter, "restart path must replace the browser WebSocket identity")
101+
} else {
102+
require.Equal(t, browserWebSocketBefore, browserWebSocketAfter, "live path must preserve the browser WebSocket identity")
103+
}
92104
})
93105
}
94106
}
95107

108+
func chromiumConfigurePowersetRestarts(bits int) bool {
109+
return bits&(matPolicy|matKioskFlags|matExtension) != 0
110+
}
111+
96112
func chromiumConfigurePowersetLabel(bits int) string {
97113
var p []string
98114
if bits&matDisplay != 0 {

0 commit comments

Comments
 (0)