Closed Bug 814078 Opened 7 years ago Closed 7 years ago

Send all queued up crash reports when we make a wifi connection

Categories

(Firefox OS Graveyard :: General, defect, P2)

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed, firefox20 fixed, b2g18 fixed)

RESOLVED FIXED
B2G C2 (20nov-10dec)
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: kairo, Assigned: hub)

References

Details

Attachments

(1 file, 3 obsolete files)

Bug 811778 makes sure we only send crash reports over wifi - from what I've seen, we queue up crash reports we couldn't send in the pending/ directory in any case. Now we need to make sure we submit those queued-up reports when we make a wifi connection.
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
blocking-basecamp: --- → ?
Assignee: nobody → hub
blocking-basecamp: ? → +
Depends on: 811778
Currently we'll purge the queue only if there was a crash report to send.

I am not sure we should check this each time we boot or get wifi connection. If we really want, it can be addressed.

I'm concerned about me adding stuff to CrashSubmit. Is it really allowed? Am I doing it right?
Attachment #684507 - Flags: feedback?(kairo)
Comment on attachment 684507 [details] [diff] [review]
Bug 814078 - Submit all the pending crash dumps.

I don't really know the code in that module, Ted or Benjamin should know if your adding of code there is OK. One of them probably needs to review those changes anyhow.
Attachment #684507 - Flags: feedback?(ted)
Attachment #684507 - Flags: feedback?(kairo)
Attachment #684507 - Flags: feedback?(benjamin)
Attachment #684507 - Flags: feedback?(benjamin)
Target Milestone: --- → B2G C2 (20nov-10dec)
Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.
Priority: -- → P2
Comment on attachment 684507 [details] [diff] [review]
Bug 814078 - Submit all the pending crash dumps.

switching to review request.
Attachment #684507 - Flags: feedback?(ted) → review?(ted)
Comment on attachment 684507 [details] [diff] [review]
Bug 814078 - Submit all the pending crash dumps.

>diff --git a/toolkit/crashreporter/CrashSubmit.jsm b/toolkit/crashreporter/CrashSubmit.jsm

>+function getAllPendingMinidumpsID() {
>+  let minidumps = [];
>+  let pendingDir = getPendingDir();
>+
>+  if (!pendingDir.isDirectory())
>+    return [];

What is the point of this check? I think that .isDirectory will throw if the path doesn't exist, so if this is a check for whether the paths exists, shouldn't we just use .exists() ?

>+  let entries = pendingDir.directoryEntries;
>+
>+  while (entries.hasMoreElements()) {
>+    let entry = entries.getNext().QueryInterface(Ci.nsIFile);
>+    if (entry.isFile()) {
>+      entry.leafName
>+      let matches = entry.leafName.match(/(.+)\.dmp$/);
>+      if (matches)
>+        minidumps.push(matches[1]);
>+    }
>+  }

This is incorrect for multi-dump reports. There will be reports of the form <ID>-browser.dmp which should not be in this list. I believe that the better thing to do here is to look for the .extra files instead of the .dmp files, because there is only one .extra file for a multi-dump report.

You can re-request review from me.
Attachment #684507 - Flags: review?(ted) → review-
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)

> >+function getAllPendingMinidumpsID() {
> >+  let minidumps = [];
> >+  let pendingDir = getPendingDir();
> >+
> >+  if (!pendingDir.isDirectory())
> >+    return [];
> 
> What is the point of this check? I think that .isDirectory will throw if the
> path doesn't exist, so if this is a check for whether the paths exists,
> shouldn't we just use .exists() ?

I actually want to be sure that 1. it exists, 2. it is a directory. I'll add for a "exists()" test at first.
 
> >+  let entries = pendingDir.directoryEntries;
> >+
> >+  while (entries.hasMoreElements()) {
> >+    let entry = entries.getNext().QueryInterface(Ci.nsIFile);
> >+    if (entry.isFile()) {
> >+      entry.leafName
> >+      let matches = entry.leafName.match(/(.+)\.dmp$/);
> >+      if (matches)
> >+        minidumps.push(matches[1]);
> >+    }
> >+  }
> 
> This is incorrect for multi-dump reports. There will be reports of the form
> <ID>-browser.dmp which should not be in this list. I believe that the better
> thing to do here is to look for the .extra files instead of the .dmp files,
> because there is only one .extra file for a multi-dump report.
> 
> You can re-request review from me.
Updated patch.

Adding Fabrice as a reviewer for shell.js
Attachment #684507 - Attachment is obsolete: true
Attachment #688919 - Flags: review?(fabrice)
Attachment #688919 - Flags: review?(benjamin)
Comment on attachment 688919 [details] [diff] [review]
Bug 814078 - Submit all the pending crash dumps.

It feels to me that the better name for this function would be "getAllPendingMinidumpIDs". r=me with that change
Attachment #688919 - Flags: review?(benjamin) → review+
r Fabrice for the changes in shell.js. Unchanged from previous version.
Attachment #688919 - Attachment is obsolete: true
Attachment #688919 - Flags: review?(fabrice)
Attachment #688934 - Flags: review?(fabrice)
Comment on attachment 688934 [details] [diff] [review]
Bug 814078 - Submit all the pending crash dumps.

Review of attachment 688934 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/chrome/content/shell.js
@@ +133,5 @@
> +
> +        // purge the queue.
> +        let pending = shell.CrashSubmit.pendingIDs();
> +        for (let crashid of pending)
> +          shell.CrashSubmit.submit(crashid);

nit: { } even for single line blocks.
Attachment #688934 - Flags: review?(fabrice) → review+
Attachment #688934 - Attachment is obsolete: true
Updated patch with nit.
Can we carry forward the r+ and land?
Yeah that's the plan. I just wanted to make sure I didn't break things. Landing in a jiffy.
Comment on attachment 689393 [details] [diff] [review]
Submit all the pending crash dumps.

[Approval Request Comment]


blocking basecamp

Note: CrashSubmit.jsm is used on desktop, I think.
Attachment #689393 - Flags: review+
Attachment #689393 - Flags: approval-mozilla-beta?
Attachment #689393 - Flags: approval-mozilla-aurora?
(the r+ was to carry forward, btw)
Hub, you usually should state both the reward and risks from taking this patch when requesting approvals. That's what the Questions are for that automatically get added when requesting, and you should post answers to those.

That said, as I have followed this one very closely, I'll do that here.


Bug caused by (feature/regressing bug #): bug 811778 (makes us send reports only via wifi, so we need to make sure we send all the B2G crash reports we recorded up to that point)
User impact if declined: Not getting crashes fixed because we don't get the data
Testing completed (on m-c, etc.): Landed on m-i, I think Hub tested on his side - we need to do B2G QA once we are on beta.
Risk to taking this patch (and alternatives if risky): This extends code that is also used for sending plugin reports on desktop, and the send functionality from about:crashes. Still, it doesn't really change those code paths so should be low risk.
String or UUID changes made by this patch: none
Also, no need to request approval for bb+ bugs :P
(In reply to Fabrice Desré [:fabrice] from comment #19)
> Also, no need to request approval for bb+ bugs :P

Erm, not even if they touch files shared with Firefox desktop and Android?
Comment on attachment 689393 [details] [diff] [review]
Submit all the pending crash dumps.

Thanks for the overview of risk to desktop. Since this will make it into b4, it should be fairly obvious if our crash volume is significantly impacted (which would be a great canary for B2G issues). Approving for Aurora/Beta.
Attachment #689393 - Flags: approval-mozilla-beta?
Attachment #689393 - Flags: approval-mozilla-beta+
Attachment #689393 - Flags: approval-mozilla-aurora?
Attachment #689393 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/fd6d1d52a2a1
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Hub - please land on Aurora/Beta today so that we can start benefiting from this work in our Nightlies. Thanks!
Flags: needinfo?(hub)
Re-opening because Marcia notes that "my 'pending' crashes are still not being submitted when connecting to my home wifi network"
Status: RESOLVED → REOPENED
QA Contact: mozillamarcia.knous
Resolution: FIXED → ---
Hub: Using yesterday's nightly build (21021212), my pending wifi reports were not sent. I was connected to two different fast wifi connections. Should I create a new bug or reopen this one? I still have the device in the same state and would appreciate any advice on what to look for in logcat.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
QA Contact: mozillamarcia.knous
Resolution: --- → FIXED
(In reply to Marcia Knous [:marcia] from comment #26)
> Hub: Using yesterday's nightly build (21021212), my pending wifi reports
> were not sent. I was connected to two different fast wifi connections.
> Should I create a new bug or reopen this one? I still have the device in the
> same state and would appreciate any advice on what to look for in logcat.

Please file a new bug.
I have to add a not that I'm not sure how well this will behave with 'captive portals' like we often find on public wifi.
I haven't had a successful pending crash submitted.  Is there a time delay in regards to checking to resend?  Is there a process that loops to check again to send?
(In reply to Naoki Hirata :nhirata from comment #30)
> I haven't had a successful pending crash submitted.  Is there a time delay
> in regards to checking to resend?  Is there a process that loops to check
> again to send?

We send the queued up reports only when there is a crash report.
(In reply to Hub Figuiere [:hub] from comment #31)
> We send the queued up reports only when there is a crash report.

What do you mean with that? We only send when we crash again and are online?

My "spec" in comment #0 clearly says we need to send all queued up reports once we get a wifi connection, so if we don't, this bug isn't fixed.
Depends on: 821498
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #32)
> (In reply to Hub Figuiere [:hub] from comment #31)
> > We send the queued up reports only when there is a crash report.
> 
> What do you mean with that? We only send when we crash again and are online?
> 
> My "spec" in comment #0 clearly says we need to send all queued up reports
> once we get a wifi connection, so if we don't, this bug isn't fixed.

If we get a crash we send it over wifi, and the queued one too. If not on wifi, sending will be delayed up to the time we get wifi.

If we crash again or reboot before getting wifi we won't send anything until we get a crash.
(In reply to Hub Figuiere [:hub] from comment #33)
> If we get a crash we send it over wifi, and the queued one too. If not on
> wifi, sending will be delayed up to the time we get wifi.

Thanks for that description, that's very understandable.

> If we crash again or reboot before getting wifi we won't send anything until
> we get a crash.

Can we fix this in bug 821498?
> Can we fix this in bug 821498?

I can check if there is a pending queue on startup and run it. Sure. I am always worry about the impact on startup perfomance.
(In reply to Hub Figuiere [:hub] from comment #35)
> > Can we fix this in bug 821498?
> 
> I can check if there is a pending queue on startup and run it. Sure. I am
> always worry about the impact on startup perfomance.

We probably should do this with a bit of delay after we make a wifi connection.
It would be done the exact same way, so yes.
(In reply to Hub Figuiere [:hub] from comment #37)
> It would be done the exact same way, so yes.

OK, so let's make bug 821498 to be about always checking for and sending pending crashes when we connect to wifi (with some delay).
You need to log in before you can comment on or make changes to this bug.