The default bug view has changed. See this FAQ.

snippets with iframes in them break the "Restore Previous Session" button on default homepage (about:home)

VERIFIED FIXED in Firefox 14

Status

()

Firefox
General
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: Yoz Grahame, Assigned: Alice0775 White)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 16
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?
in-litmus +

Firefox Tracking Flags

(firefox14+ verified, firefox15+ fixed)

Details

(Whiteboard: [about-home][qa+])

Attachments

(4 attachments, 8 obsolete attachments)

(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:7.0) Gecko/20100101 Firefox/7.0
Build ID: 20110824172139

Steps to reproduce:

1. Start FF as usual after either a clean exit or crash.
2. See default search + "Restore" button homepage.
3. Click "Restore" button.


Actual results:

Nothing. Button shows a click but the session restore doesn't happen. However, if I go to History->Restore Previous Session, that works fine.


Expected results:

Restored session as per usual. In FF6 + betas, this was working fine.
Can you repeat this reliably? If so, can you check your error console following pressing the button to see if there's anything there?
(Reporter)

Comment 2

6 years ago
Yep, repros every time. Note that I'm not using my default profile at the moment (because the default has so many tabs in it that it reliably crashes/freezes FF - maybe I should submit that elsewhere, just haven't got around to investigating yet).

Anyway, nothing appears in the Error Console on clicking. The only thing I see in the Console before then are a few extension & TestPilot-related errors & warnings, and this:

Error: Warning: unrecognized command line flag -foreground
Source File: resource:///components/nsBrowserContentHandler.js
Line: 776
Hmm. Do any dialogs pop up during startup? I know there's an issue with the default browser dialog...

Also, do you have any extensions that might be interacting?
(Reporter)

Comment 4

6 years ago
The only dialog that pops up during startup is the profile chooser - I choose my non-default profile, then the main browser window opens. (This was my startup with FF6 as well.)

I've disabled all extensions and the bug still happens, with nothing in the Error Console on click. Other possible clues in the console:

Warning: WARN addons.xpi: Add-on is invalid: Error: Directory /Applications/Firefox.app/Contents/MacOS/extensions/testpilot@labs.mozilla.com does not contain a valid install manifest
Source File: resource:///modules/XPIProvider.jsm
Line: 785

Warning: WARN addons.xpi: Could not uninstall invalid item from locked install location
Source File: resource:///modules/XPIProvider.jsm
Line: 2509

Could not read chrome manifest file '/Applications/Firefox.app/Contents/MacOS/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/chrome.manifest'.

-- these three appeared even with all extensions disabled.
I tried moving the two extension folders (testpilot@labs.mozilla.com, 		{972ce4c6-7e08-4474-a285-3208198ce6fd} ) out of /App/FF/C/MacOS/extensions/ and those three errors were replaced by this one:

Warning: WARN addons.xpi: Unable to activate the default theme
Source File: resource:///modules/XPIProvider.jsm
Line: 3206

I'm guessing that all of the above is probably irrelevant and expected, but you tell me.
(In reply to Yoz Grahame from comment #4)
> Warning: WARN addons.xpi: Add-on is invalid: Error: Directory
> /Applications/Firefox.app/Contents/MacOS/extensions/testpilot@labs.mozilla.
> com does not contain a valid install manifest
> Source File: resource:///modules/XPIProvider.jsm
> Line: 785
> 
> Warning: WARN addons.xpi: Could not uninstall invalid item from locked
> install location
> Source File: resource:///modules/XPIProvider.jsm
> Line: 2509
> 
> Could not read chrome manifest file
> '/Applications/Firefox.app/Contents/MacOS/extensions/{972ce4c6-7e08-4474-
> a285-3208198ce6fd}/chrome.manifest'.

I think those are all fine.

> -- these three appeared even with all extensions disabled.
> I tried moving the two extension folders (testpilot@labs.mozilla.com, 	
> {972ce4c6-7e08-4474-a285-3208198ce6fd} ) out of /App/FF/C/MacOS/extensions/
> and those three errors were replaced by this one:
> 
> Warning: WARN addons.xpi: Unable to activate the default theme
> Source File: resource:///modules/XPIProvider.jsm
> Line: 3206

Yea, that's unrelated, but I would put that back. Edit the profile (~/Lib/App Sup/Firefox/Profiles/), not the app bundle itself.

Let's try one more thing before I'm completely stumped... if you open a new tab and go to about:home, does the button work there?
(Reporter)

Comment 6

6 years ago
(In reply to Paul O'Shannessy [:zpao] from comment #5)

> > Warning: WARN addons.xpi: Unable to activate the default theme
> > Source File: resource:///modules/XPIProvider.jsm
> > Line: 3206
> 
> Yea, that's unrelated, but I would put that back. Edit the profile
> (~/Lib/App Sup/Firefox/Profiles/), not the app bundle itself.

I put the folders back - which file should I edit? Does it matter? (I don't mind a couple of rogue console errors if they're not breaking anything)

> Let's try one more thing before I'm completely stumped... if you open a new
> tab and go to about:home, does the button work there?

I did:
* Start FF session
* Take first tab (about:home) to a different page
* open new tab
* go about:home
* click button - again with the nothing.
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/releases/mozilla-beta/annotate/c55dabfdbc8b/browser/base/content/browser.js#l4807
In some cases,
When about:home displays completely, removeEventListener has been already executed.
So click event does not capture anymore.
(Assignee)

Comment 8

6 years ago
Created attachment 556706 [details]
broken chromeappsstore.sqlite

It was caused with corrupted chromeappsstore.sqlite.
I think that the corruption is triggered by landing of Bug 592431.
(Assignee)

Comment 9

6 years ago
s/Bug 592431/Bug 563738/
(Assignee)

Updated

6 years ago
Blocks: 563738
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Mac OS X → All
(Assignee)

Comment 10

6 years ago
WORKAROUND:
Delete chromeappsstore.sqlite and then start browser.
OR
Click "Restore Previous Session"quickly after the display of "about:home".
(Reporter)

Comment 11

6 years ago
... and as of just now, the bug is no longer repro-ing, even if I wait a few seconds before clicking. No, I didn't touch chromeappsstore.sqlite.
(Assignee)

Updated

6 years ago
Attachment #556706 - Attachment is obsolete: true
(Assignee)

Comment 12

6 years ago
(In reply to Yoz Grahame from comment #11)
> ... and as of just now, the bug is no longer repro-ing, even if I wait a few
> seconds before clicking. No, I didn't touch chromeappsstore.sqlite.
No longer blocks: 563738
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WORKSFORME
Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0

Verified WORKSFORME. @Yoz, @Alice, please reopen if you can start reproducing again.

Thanks
Status: RESOLVED → VERIFIED

Comment 14

6 years ago
We got a spike in these reports on input starting the 27/28th.  What can we do to make sure this doesn't happen again or to fix for users who hit it?
Marco, this is reportedly related to corrupt chromeappstore.sqlite. Can you comment?

Cheng, I'm purely speculating, but perhaps snippets that went out around then had bad data & triggered this?
(In reply to [:Cww] from comment #14)
> We got a spike in these reports on input starting the 27/28th.  What can we
> do to make sure this doesn't happen again or to fix for users who hit it?

For which version of Firefox do these spikes occur?

Comment 17

6 years ago
Beta. (7).  Possibly also in Aurora or nightly but we don't have as much data there.
(Assignee)

Updated

6 years ago
Duplicate of this bug: 684636
(Assignee)

Comment 19

6 years ago
Confirmed.
The problem returns.
Delleting chromeappsstore.sqlite helps.
Status: VERIFIED → REOPENED
Resolution: WORKSFORME → ---
(Assignee)

Comment 20

6 years ago
s/Delleting/Deleting/
(Assignee)

Comment 21

6 years ago
Created attachment 558216 [details]
triggered chromeappsstore.sqlite (but it will expire soon)
(Assignee)

Updated

6 years ago
Blocks: 563738

Comment 22

6 years ago
Created attachment 572001 [details]
Another chromeappstore.sqlite

Reproducing this with: Mozilla/5.0 (X11; Linux i686; rv:7.0.1) Gecko/20100101 Firefox/7.0.1

:-(
(Assignee)

Updated

6 years ago
Status: REOPENED → NEW

Comment 23

6 years ago
Created attachment 572534 [details]
The same problem experienced

The exactly same problem is being experienced.

Comment 24

6 years ago
My version is: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0.1) Gecko/20100101 Firefox/7.0.1
Removal of attached file cured the problem.
(Assignee)

Comment 25

6 years ago
Created attachment 573230 [details] [diff] [review]
patch


This problem happens if iframe is included in snippets.
I think that pagehide event from the iframe should be ignored.

Updated

6 years ago
Duplicate of this bug: 700980

Comment 27

6 years ago
Looks like we pushed another snippet this week that broke this.
The offending snippet has been disabled and won't appear anymore in a few days. If you want to reproduce this issue, you can install the snippet switcher addon: https://addons.mozilla.org/en-US/firefox/addon/home-snippets-switcher/

Set name to 'persona' and you should see the persona switcher snippet (that uses an iframe).

This kind of functional snippet is a priority for marketing, and more snippets that use iframes are coming, so can we get this fix in ASAP? Thanks!
Michael - we can figure out what needs to happen here client-side, but in the meantime the prospect of more coming seems like a non-starter if it breaks session restore functionality, no?

Also, given that our snippet QA process didn't catch this the first time around, have we added tests to ensure we don't break it again?
Correct. When I say more coming, I mean we're developing them, but we won't launch any snippets until this issue is resolved (and will only release them to versions where the issue is resolved), so that isn't an issue.

Snippets don't have a well defined QA process currently (the idea of a snippet deviating from a standard template is fairly new). Of course, this issue highlights that need, and we'll get something set up (including tests for this) for future snippets.
Asking for in-testsuite, in-litmus. Can QA get some direction on what is needed to validate this?
Flags: in-testsuite?
Flags: in-litmus?(ioana.budnar)
Hey Anthony - we don't need product QA here, exactly, but we need a QA process for the snippets that the engagement team pushes through snippets.mo, since they get injected into about:home, and can have the effect of breaking other about:home functionality, as this bug demonstrates.

Comment 33

6 years ago
(In reply to Johnathan Nightingale [:johnath] from comment #32)
> Hey Anthony - we don't need product QA here, exactly, but we need a QA
> process for the snippets that the engagement team pushes through
> snippets.mo, since they get injected into about:home, and can have the
> effect of breaking other about:home functionality, as this bug demonstrates.

I'm working to get a QA resource on snippets to help test this. Have we identified technically what exactly makes this break? Is it use of an iFrame? If so, what aspect of a iFrame + about:home + system restore functionality as iFrames are common around the web? Please advise, or I can open another bug. 

To Anthony's point there seems to be overlap between the snippet service and product here.
If you need someone from Product QA, please loop in Ioana Budnar (our QA point of contact for Session Store).
(In reply to Laura Forrest from comment #33)

> I'm working to get a QA resource on snippets to help test this. Have we
> identified technically what exactly makes this break? Is it use of an
> iFrame? If so, what aspect of a iFrame + about:home + system restore
> functionality as iFrames are common around the web? Please advise, or I can
> open another bug. 

The trick here is that the snippet is added to the page that also hosts the "Restore Previous Session" button. iframes elsewhere on the web are fine, since they live in their own pages, but snippets live in about:home, so anything they do to alter the behaviour of the page has the potential to break the other functions of about:home (session restore, and possibly search).

That suggests to me that we'd want a QA process for any snippet to ensure that serving it to users doesn't impact those functions in any way. So I agree with you that it exists at the intersection of product and snippet service, I just didn't want Anthony thinking that we had a sessionstore bug here, or something. The session store code isn't changing, but the snippet delivered to about:home is injected directly into the page, meaning it can have unintended consequences if it's not carefully checked.
What component do "snippets" fall under if not Session Store? It makes sense to me to have tests specific to snippets across the various components they may touch. In other words, tests for snippets in Search, tests for snippets in Session Store, etc.
I'm not familiar with how Product QA does tests; how would we run them against a new snippet we've developed? 

There's discussion in bug 701177 about setting up selenium tests that run against our snippet staging server. We could create a set of tests for standard about:home behavior, and then apply them to new snippets as we develop them.
I don't believe we have tests specific to About:Home (or any About: Pages for that matter) in Litmus. This might be a good place to start?

Updated

6 years ago
Component: Session Restore → General
QA Contact: session.restore → general
Whiteboard: [about-home]

Updated

6 years ago
Attachment #573230 - Attachment is patch: true
Chiming in from WebQA, here; (Please try not to read this as being defensive; I just want to provide some context and history about original coverage -- glad we're working together and talking about test strategy here now!):

* we tested and supported the deployment of the snippet-service rewrite that shipped with Firefox 4 (bug 592431 and its ilk)
* we were told that all snippets would be the simplest of valid HTML, and would only ever need, at most, link-checking and spelling/grammar eyeballing
* clearly, though, as Mike Kelly points out, <iframe> snippets are new, and need to be tested differently (doubtful that Selenium will get us as far as we need, but it could help); we'll talk about that over in bug 701177
* we're aware of "Snippet Service Improvements" and "HTML5 Snippets" as projects in planned-existence only (for now) (from https://docs.google.com/spreadsheet/ccc?key=0AhiX365xacl1dEVpSXY0Q2JSMDVaUTljaGhHXzVKOWc&hl=en_US#gid=11), and in talking to Chris More about those projects, WebQA will definitely be heavily involved--if not taking point--on testing that
Comment on attachment 573230 [details] [diff] [review]
patch

This looks like it solves the right problem, but rather than checking frameElement, I think we should instead compare event.target to aBrowser.contentDocument (or contentWindow?).
Attachment #573230 - Flags: feedback+

Comment 41

5 years ago
Testcase #45482 has been added in Litmus for:
aurora basic functional tests (bfts) (276) -> fx aurora bft - session store (2073)
aurora full functional tests (ffts) (275) -> fx aurora fft - session store (2072).

Anthony, please let me know if you want me to add the testcase to any other test suites and if you want me to also create test cases for the other about pages.
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus?(ioana.budnar)
Flags: in-litmus+
Ioana, in-testsuite is used for the automation test suites (ex. mochitest, xpcshell, etc), not for Litmus. Has there been tests for this added somewhere other than Litmus. If so, please reset the flag to in-testsuite+.

As you are owner of the Session Store tests in Litmus, I trust you to add any tests you feel are necessary. Thanks
Flags: in-testsuite+ → in-testsuite?

Comment 43

5 years ago
Anthony, thank you for the explanation. I didn't know this since it's not mentioned in the tooltip with the explanation for this flag. I couldn't find an automated testcase for this bug so the flag remains as it is.

Comment 44

5 years ago
What's the next step here? We'd like to run the Functional Personas snippet again, which was the original culprit to this bug. What do we need to do to launch that again, and not have this break? I'm not clear on the dependencies between this and Bug 701172 - any insight from the product perspective would be great.

Updated

5 years ago
Blocks: 701172
Someone needs to fix up Alice0775's patch per comment 40, and then we can land it on trunk, and potentially back-port it to Aurora and/or Beta. Given that we need to ensure that only users with this fix get the new snippet, we may have to also bump the snippet version. I think Frank is going to do that for bug 711157 anyways, so we may be able to just roll this fix up into that.

Comment 46

5 years ago
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #40)
> Comment on attachment 573230 [details] [diff] [review]
> patch
> 
> This looks like it solves the right problem, but rather than checking
> frameElement, I think we should instead compare event.target to
> aBrowser.contentDocument (or contentWindow?).

I concur. Currently, the code runs for each generated content image load too, which is bad times. :-/
Frank said he really wants to work on this.
Assignee: nobody → fryn

Comment 48

5 years ago
Created attachment 617932 [details] [diff] [review]
patch v2
Attachment #573230 - Attachment is obsolete: true
Attachment #617932 - Flags: review?(gavin.sharp)

Comment 49

5 years ago
Created attachment 617933 [details] [diff] [review]
patch v2

Typo.
Attachment #617932 - Attachment is obsolete: true
Attachment #617932 - Flags: review?(gavin.sharp)
Attachment #617933 - Flags: review?(gavin.sharp)

Comment 50

5 years ago
Created attachment 617943 [details] [diff] [review]
patch v3

s/this/aBrowser/g

excluding about:newtab, so the code isn't run on every new blank tab.
Attachment #617933 - Attachment is obsolete: true
Attachment #617933 - Flags: review?(gavin.sharp)
Attachment #617943 - Flags: review?(gavin.sharp)
Comment on attachment 617943 [details] [diff] [review]
patch v3

If we're going to want to take advantage of this fix to use more advanced snippets, we're going to need to bump the snippets version again, right?
Attachment #617943 - Flags: review?(gavin.sharp) → review+

Comment 52

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #51)
> If we're going to want to take advantage of this fix to use more advanced
> snippets, we're going to need to bump the snippets version again, right?

Yup, I bumped STARTPAGE_VERSION to 3.
Sent email to Mike Kelly in case he doesn't see this.

https://hg.mozilla.org/integration/fx-team/rev/7a5807d68ddc
Status: NEW → ASSIGNED
Whiteboard: [about-home] → [about-home][fixed-in-fx-team]

Comment 53

5 years ago
Thanks, everyone! How long until this could land in any Firefox channel?
Backed out for m-oth failures:

https://hg.mozilla.org/integration/fx-team/rev/ab8c08dfe9c4
Whiteboard: [about-home][fixed-in-fx-team] → [about-home]
https://hg.mozilla.org/mozilla-central/rev/7a5807d68ddc
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/mozilla-central/rev/ab8c08dfe9c4
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [about-home] → [about-home][backed-out]

Comment 57

5 years ago
Created attachment 626208 [details] [diff] [review]
patch v4

Patch v3 failed, because contentDocument's getter can throw.

Checking frameElement doesn't work, because it returns null during pagehide for iframes.

We'll also adding the listeners way too often, including for all about:blank and about:newtab documents. I fixed that.

onStateChange hits that `if branch` twice for each applicable document, adding the listeners an extra time. I don't know why that happens or how best to fix it. It doesn't break anything, because addEventListener avoids adding functions twice, so BrowserOnClick only gets added once, but it's still bad behavior. :/
Attachment #617943 - Attachment is obsolete: true
Attachment #626208 - Flags: review?(gavin.sharp)
Comment on attachment 626208 [details] [diff] [review]
patch v4

>-    if (/^about:certerror/.test(ownerDoc.documentURI)) {
>+    if (/^about:certerror/i.test(ownerDoc.documentURI)) {

>-    else if (/^about:blocked/.test(ownerDoc.documentURI)) {
>+    else if (/^about:blocked/i.test(ownerDoc.documentURI)) {

These changes are unnecessary. Those URIs are only loaded internally, there's no reason for a user to type them.

Comment 59

5 years ago
(In reply to Dão Gottwald [:dao] from comment #58)
> Comment on attachment 626208 [details] [diff] [review]
> patch v4
> 
> >-    if (/^about:certerror/.test(ownerDoc.documentURI)) {
> >+    if (/^about:certerror/i.test(ownerDoc.documentURI)) {
> 
> >-    else if (/^about:blocked/.test(ownerDoc.documentURI)) {
> >+    else if (/^about:blocked/i.test(ownerDoc.documentURI)) {
> 
> These changes are unnecessary. Those URIs are only loaded internally,
> there's no reason for a user to type them.

Fair, and I knew. I was just making the `if` conditions consistent with each other. I can remove them.
Status: REOPENED → ASSIGNED
(In reply to Frank Yan (:fryn) from comment #57)
> Checking frameElement doesn't work, because it returns null during pagehide
> for iframes.

That doesn't sound right. Are you sure? Both .top and .frameElement just use the docshell hierachy - if one doesn't work I'd expect the other doesn't work either.

Comment 61

5 years ago
Hi everyone. I need to check on the on the ETA for this bug? This is blocking the Engagement team from doing Snippets that contain iframes and specific use-cases for persona/add-on snippets. Is this bug a priority and when could it land on a Firefox Channel?
Assignee: fryn → nobody
Status: ASSIGNED → NEW
Component: General → File Handling
QA Contact: general → file.handling
Summary: "Restore Previous Session" button on default homepage does nothing → snippets with iframes in them break the "Restore Previous Session" button on default homepage (about:home)
Target Milestone: --- → Firefox 14
Assignee: nobody → fryn
Component: File Handling → General
QA Contact: file.handling → general
Target Milestone: Firefox 14 → ---

Comment 62

5 years ago
Created attachment 632914 [details] [diff] [review]
patch v5

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #60)
> Both .top and .frameElement just use
> the docshell hierachy - if one doesn't work I'd expect the other doesn't
> work either.

You're right.
Attachment #626208 - Attachment is obsolete: true
Attachment #626208 - Flags: review?(gavin.sharp)
Attachment #632914 - Flags: review?(gavin.sharp)

Comment 63

5 years ago
Created attachment 632922 [details] [diff] [review]
patch v5

Breaking the patch into two pieces to reduce risk in case we need to uplift this to aurora and/or beta.
Attachment #632914 - Attachment is obsolete: true
Attachment #632914 - Flags: review?(gavin.sharp)
Attachment #632922 - Flags: review?(gavin.sharp)

Comment 64

5 years ago
Created attachment 632923 [details] [diff] [review]
patch v5: part 2
Attachment #632923 - Flags: review?(gavin.sharp)
Attachment #632922 - Flags: review?(gavin.sharp) → review+
Can we write a test that covers this case?

Updated

5 years ago
Assignee: fryn → alice0775
Hardware: x86 → All
Whiteboard: [about-home][backed-out] → [about-home]
Target Milestone: --- → Firefox 16
Version: 7 Branch → Trunk

Comment 66

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c25ccddcf421
Status: NEW → ASSIGNED

Updated

5 years ago
Attachment #632922 - Attachment description: patch v5: part 1 → patch v5
Attachment #632922 - Attachment filename: abouthomefixrestore1 → abouthomefixrestore

Updated

5 years ago
Attachment #632923 - Attachment filename: abouthomefixrestore → abouthomefixrestorepart2
Attachment #632923 - Attachment is obsolete: true
Attachment #632923 - Flags: review?(gavin.sharp)

Updated

5 years ago
Blocks: 764989

Updated

5 years ago
Blocks: 764991
https://hg.mozilla.org/mozilla-central/rev/c25ccddcf421
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Comment 68

5 years ago
Does anyone know how to get the patch uplifted to channel closer to release sooner than waiting for all of the trains? There are some interactive snippets that we want to do around Firefox Desktop retention and this has been blocking us for many months.
(In reply to Chris More [:cmore] from comment #68)
> Does anyone know how to get the patch uplifted to channel closer to release
> sooner than waiting for all of the trains? There are some interactive
> snippets that we want to do around Firefox Desktop retention and this has
> been blocking us for many months.

Gavin, what do you think about the risk of uplift to, say, Aurora? We are early in the 15 aurora cycle, and this is leaf-node code, but it's very high-traffic leaf node code.

(Nominating so that rel mgmt has it on FF15 radar, too)
tracking-firefox15: --- → ?
Comment on attachment 632922 [details] [diff] [review]
patch v5

[Triage Comment]
Sure, I think landing this on Aurora would be fine.
Attachment #632922 - Flags: approval-mozilla-aurora+
status-firefox15: --- → affected
tracking-firefox15: ? → +
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #70)
> Comment on attachment 632922 [details] [diff] [review]
> patch v5
> 
> [Triage Comment]
> Sure, I think landing this on Aurora would be fine.

Someone's going to ask you the beta question, so let's go ahead and anticipate that now, too, while we're at it. (That is, anticipate it by considering and responding - not by blanket approving if you think that's a bad idea).
It'd be nice to confirm that the fix is effective before we land on beta (or aurora, really).

Chris, is there a snippets-stage/test/whatever url that will send the "interactive snippets" needed to verify this?

Comment 73

5 years ago
Gavin: Let me enable a snippet on stage that has an iframe and I will give you a URL to test. You will need to use the snippet switch add-on or just manually edit the LocalStorage value to point at snippets.stage.mozilla.com.

Comment 74

5 years ago
Gavin: Follow the steps below to test a personal snippet.

1) Get the snippets switcher add-on here: https://github.com/mozilla/home-snippets-switcher

2) Change the snippets URL to snippets.stage.mozilla.com

3) Change the name field from %NAME% to personas.

4) Refresh about:home

It should load this URL and snippets into about:home: https://snippets.stage.mozilla.com/1/personas/14.0/20120612164001/Darwin_Universal-gcc3/en-US/beta/Darwin%2010.8.0/default/default/

Comment 75

5 years ago
:johnath/:gavin : I just need confirmation on what channel this is going to be uplifted to and what train it will land on for the July 17th release?
Comment on attachment 632922 [details] [diff] [review]
patch v5

I think taking this on beta would be fine, but we need someone to do the testing in comment 74.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): exists since about:home was added
User impact if declined: producing "advanced" snippets with iframes will break buttons on about:home
Testing completed (on m-c, etc.): since it depends on snippet service serving a certain type of snippets, need to test manually - see comment 74
Risk to taking this patch (and alternatives if risky): pretty low risk. will only affect the case where these advanced snippets are displayed
String or UUID changes made by this patch: none
Attachment #632922 - Flags: approval-mozilla-beta?
Chris: assuming we take this on beta, the fix will be included with Firefox 14, released on July 17.
Comment on attachment 632922 [details] [diff] [review]
patch v5

approving and will add qawanted to verify with the steps in comment 74
Attachment #632922 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Keywords: qawanted
Frank, can you take care of the aurora/beta landings?
status-firefox14: --- → affected
tracking-firefox14: --- → +
Marking this verified fixed using the test in comment 74 against Firefox 16.0a1 2012-06-25.
Status: RESOLVED → VERIFIED
Keywords: qawanted
Whiteboard: [about-home] → [about-home][qa+]

Comment 81

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #79)
> Frank, can you take care of the aurora/beta landings?

Done.
https://hg.mozilla.org/releases/mozilla-aurora/rev/d307a92fdcdf
https://hg.mozilla.org/releases/mozilla-beta/rev/e421095549ad
status-firefox14: affected → fixed
status-firefox15: affected → fixed

Comment 82

5 years ago
Verified as fixed on Firefox 14.0 beta 12 (20120710123126) on Windows XP, Ubuntu 12.04 and Mac OS X 10.8. The Home page Session restore button works as expected.
status-firefox14: fixed → verified
You need to log in before you can comment on or make changes to this bug.