Delay snippets initialization until delayed startup.

RESOLVED FIXED in Firefox 29

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

unspecified
Firefox 30
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 fixed, firefox30 fixed, fennec29+)

Details

Attachments

(2 attachments)

Bug 964510 is adding a delayed startup notification. We should use it for snippets.
Posted patch PatchSplinter Review
Attachment #8366481 - Flags: review?(margaret.leibovic)
Comment on attachment 8366481 [details] [diff] [review]
Patch

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

::: mobile/android/components/Snippets.js
@@ +323,5 @@
>      switch(topic) {
>        case "profile-after-change":
> +        Services.obs.addObserver(this, "browser-delayed-startup-finished", false);
> +        break;
> +      case "browser-delayed-startup-finished":

Can't you just change the category here to listen for browser-delayed-startup instead?
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/MobileComponents.manifest?force=1#107
Comment on attachment 8366481 [details] [diff] [review]
Patch

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

The answer appears to be no, so I'm fine with this approach.
Attachment #8366481 - Flags: review?(margaret.leibovic) → review+
Hi, sorry backedout as part of https://tbpl.mozilla.org/?tree=Fx-Team&rev=c15e29ead092 for the suspicion of causing robocop testfailures on android like https://tbpl.mozilla.org/php/getParsedLog.php?id=33734468&tree=Fx-Team
https://hg.mozilla.org/mozilla-central/rev/ed28aeea074a
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
backed this out again for suspicion of a startup regression:
https://hg.mozilla.org/integration/fx-team/rev/1b5dfc70d6cd
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
We should try re-landing something like this before re-enabling snippets, since it seems like that was guilty of eideticker regressions, even with bug 969511.

I can take this over. I think we should write a patch that re-adds the delayed start-up notification, but only use it for snippets at first, not a bunch of different things that might cause regressions.
Assignee: wjohnston → nobody
Blocks: 962349
Assignee: nobody → margaret.leibovic
Actually, scratch that, there's not actually any new patch to write here. We just need to land bug 964510 by itself (no consumers), make sure that doesn't cause a regression, then try landing this patch by itself. Although that shouldn't do anything until bug 962349 re-lands as well.
Assignee: margaret.leibovic → wjohnston
tracking-fennec: --- → ?
Depends on: 964510
tracking-fennec: ? → 29+
No regression from bug 964510 \o/ (if there were, I would be really confused).

So, shall we try re-landing this? However, this won't actually do anything until snippets are enabled, so maybe we should re-land bug 962349 at the same time and hope for the best?
Flags: needinfo?(wjohnston)
https://hg.mozilla.org/mozilla-central/rev/9e58486f1658
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 29 → Firefox 30
Eideticker is looking pretty stable still, so I think we should uplift this bug and bug 964510 to aurora, since snippets are still enabled there. Wes, do you want to nom them?
Comment on attachment 8366481 [details] [diff] [review]
Patch

Sure. The rel-drivers, this is a bit confusing. This stuff landed in 29, but was then backed out. Now that we know it wasn't the cause of the regression, we'd like to put it back in.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 962349
User impact if declined: Startup regression
Testing completed (on m-c, etc.): Has been on mc for a week. Things seem better
Risk to taking this patch (and alternatives if risky): really low risk. we can't really ship with snippets disabled anymore, so we don't have any good alternatives.
String or IDL/UUID changes made by this patch: none
Attachment #8366481 - Flags: approval-mozilla-aurora?
Flags: needinfo?(wjohnston)
Attachment #8366481 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
There's nothing to re-land on Aurora. This never got backed out from 29.
This component still gets initialized at startup just so it can connect to the browser-delayed-startup-finished event.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8450236 - Flags: review?(wjohnston) → review+
Comment on attachment 8450236 [details] [diff] [review]
Delay Snippets.js initialization until after startup

I like this. Seems safe.
Attachment #8450236 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/5a1db80a4c08
Status: REOPENED → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → FIXED
This follow-up really should have gone in a new bug, since the tracking flags for this bug are now confusing.

Running a local build, I'm seeing this error:

[JavaScript Error: "NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]" {file: "jar:jar:file:///data/app/org.mozilla.fennec_leibovic-1.apk!/assets/omni.ja!/components/Snippets.js" line: 389}]

Looks like you forgot to remove this line:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/Snippets.js#389
Depends on: 1042502
You need to log in before you can comment on or make changes to this bug.