Closed Bug 1214408 Opened 5 years ago Closed 5 years ago

[Session Restore] Telemetry on SessionStore:update OOM

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox44 --- affected
firefox45 --- fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(1 file)

No description provided.
Assignee: nobody → dteller
Bug 1214408 - Telemetry on SessionStore:update OOM;r?ttaubert
Attachment #8674531 - Flags: review?(ttaubert)
Attachment #8674531 - Flags: review?(ttaubert) → review+
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
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.
Attachment #8674531 - Flags: review+
(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 ...|.
Attachment #8674531 - Flags: review?(ttaubert) → review+
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
Attachment #8674531 - Attachment description: MozReview Request: Bug 1214408 - Telemetry on SessionStore:update OOM;r?ttaubert → MozReview Request: Bug 1214408 - Telemetry on SessionStore:update OOM;r=ttaubert
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
Flags: needinfo?(dteller)
Keywords: checkin-needed
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.
Flags: needinfo?(dteller)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8fea00e5b778
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
You need to log in before you can comment on or make changes to this bug.