1321593 - Refactor nsXREAppData so that Firefox -app and builtin share more codepaths, less manual memory management

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Benjamin Smedberg]

Spinoff from bug 1306327 comment 7. Right now the normal Firefox startup path and the -app startup path are separate in annoying ways. Unifying those codepaths will make things better.

Comment 1

[Benjamin Smedberg]

Attachment: Bug 1321593 part B - Unify the startup path between `firefox --app` and normal Firefox startup,

* Rename nsXREAppData to XREAppData, and make it strongly own references.
* Separate the compiled-in appdata into its own class "StaticXREAppData"
* Remove the owner/helper class ScopedAppData and related SetAllocatedString helpers
* Remove the nsXREAppData.size field which is no longer relevant as we don't support cross-version embedding
* Remove XRE_CreateAppData and XRE_FreeAppData helpers which are no longer necessary

Review commit: https://reviewboard.mozilla.org/r/96954/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96954/

Comment 2

[Mike Hommey [:glandium]]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #1)
> Created attachment 8816187 [details]
> Bug 1321593 - Unify the startup paths between normal Firefox startup and
> firefox -app startup. . As part of this change, this reorganizes the
> nsXREAppData and ScopedAppData classes:
> 
> * Rename nsXREAppData to XREAppData, and make it strongly own references.
> * Separate the compiled-in appdata into its own class "StaticXREAppData"
> * Remove the owner/helper class ScopedAppData and related SetAllocatedString
> helpers
> * Remove the nsXREAppData.size field which is no longer relevant as we don't
> support cross-version embedding
> * Remove XRE_CreateAppData and XRE_FreeAppData helpers which are no longer
> necessary

Can I ask you to make each of those in separate patches? You don't need to do that in separate bugs, just separate patches.

Comment 3

[Benjamin Smedberg]

I can separate out the -app/nsBrowserApp bits, but I don't think I can separate the other refactorings in a useful way. createappdata/freeappdata for example are tied to the ownership model, which changes at the same time.

Comment 4

[Benjamin Smedberg]

Attachment: Bug 1321593 part A - Refactor nsXREAppData: 1) make nsXREAppData strongly own its members 2) rename it to mozilla::XREAppData 3) separate out the static compiled data into StaticXREAppData 4) Remove XRE_CreateAppData and XRE_FreeAppData,

Review commit: https://reviewboard.mozilla.org/r/97196/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97196/

Comment 5

[Benjamin Smedberg]

Comment on attachment 8816187 [details]
Bug 1321593 part B - Unify the startup path between `firefox --app` and normal Firefox startup,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96954/diff/1-2/

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8816447 [details]
Bug 1321593 part A - Refactor nsXREAppData: 1) make nsXREAppData strongly own its members 2) rename it to mozilla::XREAppData 3) separate out the static compiled data into StaticXREAppData 4) Remove XRE_CreateAppData and XRE_FreeAppData,

https://reviewboard.mozilla.org/r/97196/#review98024

FWIW, I don't see an obvious reason why this couldn't have been split in smaller pieces. That would have made the review much easier.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8816187 [details]
Bug 1321593 part B - Unify the startup path between `firefox --app` and normal Firefox startup,

https://reviewboard.mozilla.org/r/96954/#review98026

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eae2252a519f
part A - Refactor nsXREAppData: 1) make nsXREAppData strongly own its members 2) rename it to mozilla::XREAppData 3) separate out the static compiled data into StaticXREAppData 4) Remove XRE_CreateAppData and XRE_FreeAppData 5) remove the struct size and related size-checking code which was only ever useful for cross-version compatibility, r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/e531af57cd60
part B - Unify the startup path between `firefox --app` and normal Firefox startup, r=glandium

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for leaks, e.g. in clipboard and jetpack tests, at least on Linux x64 debug:

https://hg.mozilla.org/integration/mozilla-inbound/rev/34405a5f3ec29f32e2c8a44132552693cbd14118
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ca33efe1b83fbc8968f59c87f7485710d82eb69

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e531af57cd60ba243721c90fc30fa47d6bdd37e0
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=40495944&repo=mozilla-inbound

> TEST-UNEXPECTED-FAIL | leakcheck | default process: 200 bytes leaked (nsLocalFile, nsStringBuffer)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0e38abdcdd8
part A - Refactor nsXREAppData: 1) make nsXREAppData strongly own its members 2) rename it to mozilla::XREAppData 3) separate out the static compiled data into StaticXREAppData 4) Remove XRE_CreateAppData and XRE_FreeAppData 5) remove the struct size and related size-checking code which was only ever useful for cross-version compatibility, r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/620f6fd44770
part B - Unify the startup path between `firefox --app` and normal Firefox startup. With fixup to properly release/not-leak xreDirectory r=glandium

Comment 11

[Benjamin Smedberg]

It was a "simple" refcount leak in nsBrowserApp, fixed with a COMPtr.

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a0e38abdcdd8
https://hg.mozilla.org/mozilla-central/rev/620f6fd44770