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

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?

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

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

Review ping?

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

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.

(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 on attachment 8674531 [details] MozReview Request: Bug 1214408 - Telemetry on SessionStore:update OOM;r=ttaubert Bug 1214408 - Telemetry on SessionStore:update OOM;r?ttaubert

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.

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

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 ...|.

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 on attachment 8674531 [details] MozReview Request: Bug 1214408 - Telemetry on SessionStore:update OOM;r=ttaubert Bug 1214408 - Telemetry on SessionStore:update OOM;r=ttaubert

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

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 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 on attachment 8674531 [details] MozReview Request: Bug 1214408 - Telemetry on SessionStore:update OOM;r=ttaubert Bug 1214408 - Telemetry on SessionStore:update OOM;r=ttaubert

Rebased.

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