Last Comment Bug 682944 - snippets with iframes in them break the "Restore Previous Session" button on default homepage (about:home)
: snippets with iframes in them break the "Restore Previous Session" button on ...
Status: VERIFIED FIXED
[about-home][qa+]
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Firefox 16
Assigned To: Alice0775 White
:
Mentors:
: 684636 700980 (view as bug list)
Depends on:
Blocks: 764991 563738 701172 764989
  Show dependency treegraph
 
Reported: 2011-08-29 12:59 PDT by Yoz Grahame
Modified: 2013-11-12 00:57 PST (History)
27 users (show)
anthony.s.hughes: in‑testsuite?
ioana_damy: in‑litmus+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
fixed


Attachments
broken chromeappsstore.sqlite (928.00 KB, text/plain)
2011-08-29 16:23 PDT, Alice0775 White
no flags Details
triggered chromeappsstore.sqlite (but it will expire soon) (160.00 KB, text/plain)
2011-09-04 20:56 PDT, Alice0775 White
no flags Details
Another chromeappstore.sqlite (128.00 KB, text/plain)
2011-11-04 09:22 PDT, :Gijs Kruitbosch (away 26-29 incl.)
no flags Details
The same problem experienced (160.00 KB, text/plain)
2011-11-07 10:54 PST, Vladik
no flags Details
patch (1.17 KB, patch)
2011-11-09 09:19 PST, Alice0775 White
gavin.sharp: feedback+
Details | Diff | Splinter Review
patch v2 (205 bytes, patch)
2012-04-24 10:34 PDT, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
patch v2 (1.58 KB, patch)
2012-04-24 10:35 PDT, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
patch v3 (1.61 KB, patch)
2012-04-24 10:45 PDT, Frank Yan (:fryn)
gavin.sharp: review+
Details | Diff | Splinter Review
patch v4 (4.97 KB, patch)
2012-05-22 15:20 PDT, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
patch v5 (4.16 KB, patch)
2012-06-13 15:36 PDT, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review
patch v5 (2.24 KB, patch)
2012-06-13 15:49 PDT, Frank Yan (:fryn)
gavin.sharp: review+
gavin.sharp: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
patch v5: part 2 (3.10 KB, patch)
2012-06-13 15:50 PDT, Frank Yan (:fryn)
no flags Details | Diff | Splinter Review

Description Yoz Grahame 2011-08-29 12:59:19 PDT
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.
Comment 1 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-08-29 13:01:27 PDT
Can you repeat this reliably? If so, can you check your error console following pressing the button to see if there's anything there?
Comment 2 Yoz Grahame 2011-08-29 13:13:34 PDT
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
Comment 3 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-08-29 13:38:14 PDT
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?
Comment 4 Yoz Grahame 2011-08-29 13:58:34 PDT
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.
Comment 5 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-08-29 14:05:17 PDT
(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?
Comment 6 Yoz Grahame 2011-08-29 14:48:39 PDT
(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.
Comment 7 Alice0775 White 2011-08-29 15:51:39 PDT
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.
Comment 8 Alice0775 White 2011-08-29 16:23:12 PDT
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.
Comment 9 Alice0775 White 2011-08-29 16:26:27 PDT
s/Bug 592431/Bug 563738/
Comment 10 Alice0775 White 2011-08-29 17:51:15 PDT
WORKAROUND:
Delete chromeappsstore.sqlite and then start browser.
OR
Click "Restore Previous Session"quickly after the display of "about:home".
Comment 11 Yoz Grahame 2011-08-30 21:08:51 PDT
... 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.
Comment 12 Alice0775 White 2011-08-30 21:29:47 PDT
(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.
Comment 13 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-08-30 23:11:26 PDT
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
Comment 14 [:Cww] 2011-09-01 10:55:09 PDT
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?
Comment 15 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-09-01 11:21:51 PDT
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?
Comment 16 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-01 11:49:35 PDT
(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 [:Cww] 2011-09-01 16:32:14 PDT
Beta. (7).  Possibly also in Aurora or nightly but we don't have as much data there.
Comment 18 Alice0775 White 2011-09-04 20:52:42 PDT
*** Bug 684636 has been marked as a duplicate of this bug. ***
Comment 19 Alice0775 White 2011-09-04 20:54:00 PDT
Confirmed.
The problem returns.
Delleting chromeappsstore.sqlite helps.
Comment 20 Alice0775 White 2011-09-04 20:54:30 PDT
s/Delleting/Deleting/
Comment 21 Alice0775 White 2011-09-04 20:56:17 PDT
Created attachment 558216 [details]
triggered chromeappsstore.sqlite (but it will expire soon)
Comment 22 :Gijs Kruitbosch (away 26-29 incl.) 2011-11-04 09:22:53 PDT
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

:-(
Comment 23 Vladik 2011-11-07 10:54:57 PST
Created attachment 572534 [details]
The same problem experienced

The exactly same problem is being experienced.
Comment 24 Vladik 2011-11-07 11:01:41 PST
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.
Comment 25 Alice0775 White 2011-11-09 09:19:11 PST
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.
Comment 26 [:Cww] 2011-11-09 10:36:12 PST
*** Bug 700980 has been marked as a duplicate of this bug. ***
Comment 27 [:Cww] 2011-11-09 10:54:52 PST
Looks like we pushed another snippet this week that broke this.
Comment 28 Michael Kelly [:mkelly,:Osmose] 2011-11-09 11:16:19 PST
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!
Comment 29 Johnathan Nightingale [:johnath] 2011-11-09 13:09:50 PST
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?
Comment 30 Michael Kelly [:mkelly,:Osmose] 2011-11-09 13:22:01 PST
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.
Comment 31 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-09 13:26:32 PST
Asking for in-testsuite, in-litmus. Can QA get some direction on what is needed to validate this?
Comment 32 Johnathan Nightingale [:johnath] 2011-11-09 13:36:25 PST
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 Laura Forrest 2011-11-09 13:43:14 PST
(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.
Comment 34 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-09 13:46:02 PST
If you need someone from Product QA, please loop in Ioana Budnar (our QA point of contact for Session Store).
Comment 35 Johnathan Nightingale [:johnath] 2011-11-09 14:15:30 PST
(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.
Comment 36 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-09 14:19:08 PST
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.
Comment 37 Michael Kelly [:mkelly,:Osmose] 2011-11-09 16:46:08 PST
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.
Comment 38 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-09 19:21:59 PST
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?
Comment 39 Stephen Donner [:stephend] 2011-11-11 03:06:44 PST
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 40 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-13 15:54:50 PST
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?).
Comment 41 Ioana (away) 2012-01-27 09:27:53 PST
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.
Comment 42 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-01-27 10:19:14 PST
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
Comment 43 Ioana (away) 2012-01-30 04:53:46 PST
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 Laura Forrest 2012-02-29 14:46:56 PST
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.
Comment 45 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-04 17:40:08 PST
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 Frank Yan (:fryn) 2012-04-09 15:50:52 PDT
(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. :-/
Comment 47 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-04-09 16:00:22 PDT
Frank said he really wants to work on this.
Comment 48 Frank Yan (:fryn) 2012-04-24 10:34:22 PDT
Created attachment 617932 [details] [diff] [review]
patch v2
Comment 49 Frank Yan (:fryn) 2012-04-24 10:35:38 PDT
Created attachment 617933 [details] [diff] [review]
patch v2

Typo.
Comment 50 Frank Yan (:fryn) 2012-04-24 10:45:38 PDT
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.
Comment 51 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-14 16:34:38 PDT
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?
Comment 52 Frank Yan (:fryn) 2012-05-14 18:15:20 PDT
(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
Comment 53 Chris More [:cmore] 2012-05-14 21:21:06 PDT
Thanks, everyone! How long until this could land in any Firefox channel?
Comment 54 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-05-14 23:45:28 PDT
Backed out for m-oth failures:

https://hg.mozilla.org/integration/fx-team/rev/ab8c08dfe9c4
Comment 55 Rob Campbell [:rc] (:robcee) 2012-05-16 13:15:09 PDT
https://hg.mozilla.org/mozilla-central/rev/7a5807d68ddc
Comment 56 Rob Campbell [:rc] (:robcee) 2012-05-16 13:16:01 PDT
https://hg.mozilla.org/mozilla-central/rev/ab8c08dfe9c4
Comment 57 Frank Yan (:fryn) 2012-05-22 15:20:04 PDT
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. :/
Comment 58 Dão Gottwald [:dao] 2012-05-23 01:33:58 PDT
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 Frank Yan (:fryn) 2012-05-23 02:49:57 PDT
(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.
Comment 60 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-25 11:41:22 PDT
(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 Chris More [:cmore] 2012-06-12 14:49:32 PDT
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?
Comment 62 Frank Yan (:fryn) 2012-06-13 15:36:17 PDT
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.
Comment 63 Frank Yan (:fryn) 2012-06-13 15:49:24 PDT
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.
Comment 64 Frank Yan (:fryn) 2012-06-13 15:50:00 PDT
Created attachment 632923 [details] [diff] [review]
patch v5: part 2
Comment 65 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-13 18:15:25 PDT
Can we write a test that covers this case?
Comment 67 Ed Morley [:emorley] 2012-06-15 05:58:27 PDT
https://hg.mozilla.org/mozilla-central/rev/c25ccddcf421
Comment 68 Chris More [:cmore] 2012-06-15 08:41:59 PDT
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.
Comment 69 Johnathan Nightingale [:johnath] 2012-06-15 10:05:43 PDT
(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)
Comment 70 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-15 16:50:04 PDT
Comment on attachment 632922 [details] [diff] [review]
patch v5

[Triage Comment]
Sure, I think landing this on Aurora would be fine.
Comment 71 Johnathan Nightingale [:johnath] 2012-06-18 07:28:15 PDT
(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).
Comment 72 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-18 14:56:55 PDT
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 Chris More [:cmore] 2012-06-18 15:26:18 PDT
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 Chris More [:cmore] 2012-06-18 15:46:57 PDT
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 Chris More [:cmore] 2012-06-20 12:47:05 PDT
: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 76 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-21 16:31:43 PDT
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
Comment 77 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-21 16:36:01 PDT
Chris: assuming we take this on beta, the fix will be included with Firefox 14, released on July 17.
Comment 78 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-22 15:13:58 PDT
Comment on attachment 632922 [details] [diff] [review]
patch v5

approving and will add qawanted to verify with the steps in comment 74
Comment 79 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-22 22:17:29 PDT
Frank, can you take care of the aurora/beta landings?
Comment 80 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-25 15:59:24 PDT
Marking this verified fixed using the test in comment 74 against Firefox 16.0a1 2012-06-25.
Comment 81 Frank Yan (:fryn) 2012-06-25 18:27:20 PDT
(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
Comment 82 Ioana (away) 2012-07-12 07:48:15 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.