1214408 - [Session Restore] Telemetry on SessionStore:update OOM

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: Bug 1214408 - Telemetry on SessionStore:update OOM;r=ttaubert

Bug 1214408 - Telemetry on SessionStore:update OOM;r?ttaubert

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8674531 [details]
MozReview Request: Bug 1214408 - Telemetry on SessionStore:update OOM;r=ttaubert

https://reviewboard.mozilla.org/r/22239/#review19887

r=me with some manual testing that the OOM error is properly recorded.

::: browser/components/sessionstore/SessionStore.jsm:795
(Diff revision 1)
> +        SessionStoreInternal.reportInternalError(data);

this.reportInternalError(data);

::: browser/components/sessionstore/content/content-sessionStore.js:717
(Diff revision 1)
> +    } catch (ex if
> +      (ex == Components.result.NS_ERROR_OUT_OF_MEMORY)
> +    ||(ex && typeof ex == "object" && ex.result == Components.result.NS_ERROR_OUT_OF_MEMORY)

That's not super pretty... can exceptions really have those two forms? If we need both then maybe do:

try {
 ... 
} catch (ex if isOOMError(ex)) {
 ...
}

::: browser/components/sessionstore/content/content-sessionStore.js:724
(Diff revision 1)
> +      sendMessage("SessionStore:error"), {
> +        telemetry
> +      });

This isn't valid syntax, did you run tests and try that this sends errors to the parent as expected?

Comment 3

[David Teller [:Yoric] - still alive but not very active]

I admit I've been lazy with this first patch. I'll try and reproduce the OOM in a test, this should help.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8674531 [details]
MozReview Request: Bug 1214408 - Telemetry on SessionStore:update OOM;r=ttaubert

Bug 1214408 - Telemetry on SessionStore:update OOM;r?ttaubert

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Review ping?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Well if you set the r? flag then I'll see it in my dashboard :) Will take a look now.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8674531 [details]
MozReview Request: Bug 1214408 - Telemetry on SessionStore:update OOM;r=ttaubert

https://reviewboard.mozilla.org/r/22239/#review20989

It's great that you spent the time writing a test (which I think you forgot to 'hg add' btw) but I'm not a fan of such amounts of test functions in production code.

Can we somehow from the outside, maybe from another content script, override sendMessage? Otherwise I would prefer not landing a test (or rather not landing those test functions) and seeing whether the crash numbers drop.

::: browser/components/sessionstore/content/content-sessionStore.js:752
(Diff revisions 1 - 2)
> -    } catch (ex if
> +    } catch (ex if typeof ex == "object" && ex.result == Components.results.NS_ERROR_OUT_OF_MEMORY) {

Can we do:

} catch (ex if ex && ex.result == Components.results.NS_ERROR_OUT_OF_MEMORY) {

Should work just as fine even if it's not an object.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

(In reply to Tim Taubert [:ttaubert] from comment #6)
> Well if you set the r? flag then I'll see it in my dashboard :) Will take a
> look now.

Yeah, I hadn't realized that MozReview had turned it into a r+.
Thanks for the review.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8674531 [details]
MozReview Request: Bug 1214408 - Telemetry on SessionStore:update OOM;r=ttaubert

Bug 1214408 - Telemetry on SessionStore:update OOM;r?ttaubert

Comment 10

[David Teller [:Yoric] - still alive but not very active]

https://reviewboard.mozilla.org/r/22239/#review20989

> Can we do:
> 
> } catch (ex if ex && ex.result == Components.results.NS_ERROR_OUT_OF_MEMORY) {
> 
> Should work just as fine even if it's not an object.

Ah, good to know.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Ok, I managed to inject custom `sendSyncMessage`/`sendAsyncMessage` into the frame script. Ugly, but good to know.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8674531 [details]
MozReview Request: Bug 1214408 - Telemetry on SessionStore:update OOM;r=ttaubert

https://reviewboard.mozilla.org/r/22239/#review21141

::: browser/components/sessionstore/content/content-sessionStore.js:713
(Diff revisions 2 - 3)
> -    } catch (ex if typeof ex == "object" && ex.result == Components.results.NS_ERROR_OUT_OF_MEMORY) {
> +    } catch (ex if ex.result == Cr.NS_ERROR_OUT_OF_MEMORY) {

This fails if ex==null (which probably can happen?). Should be |ex if ex && ex.result ...|.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

https://reviewboard.mozilla.org/r/22239/#review21141

> This fails if ex==null (which probably can happen?). Should be |ex if ex && ex.result ...|.

The semantics of this `if` are... interesting.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8674531 [details]
MozReview Request: Bug 1214408 - Telemetry on SessionStore:update OOM;r=ttaubert

Bug 1214408 - Telemetry on SessionStore:update OOM;r=ttaubert

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=597879fb26cc

Comment 16

[Carsten Book [:Tomcat]]

failed to apply:

patching file toolkit/components/telemetry/Histograms.json
Hunk #1 FAILED at 4301
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/telemetry/Histograms.json.rej
patch failed to apply
abort: fix up the merge and run hg transplant --continue

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8674531 [details]
MozReview Request: Bug 1214408 - Telemetry on SessionStore:update OOM;r=ttaubert

Bug 1214408 - Telemetry on SessionStore:update OOM;r=ttaubert

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8674531 [details]
MozReview Request: Bug 1214408 - Telemetry on SessionStore:update OOM;r=ttaubert

Bug 1214408 - Telemetry on SessionStore:update OOM;r=ttaubert

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Rebased.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8fea00e5b778

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8fea00e5b778

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/8fea00e5b778

Comment 23

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion