Closed
Bug 682944
Opened 13 years ago
Closed 13 years ago
snippets with iframes in them break the "Restore Previous Session" button on default homepage (about:home)
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox 16
People
(Reporter: yoz, Assigned: alice0775)
References
(Blocks 1 open bug)
Details
(Whiteboard: [about-home][qa+])
Attachments
(4 files, 8 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Gavin
:
review+
Gavin
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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•13 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
Comment 3•13 years ago
|
||
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•13 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.
Comment 5•13 years ago
|
||
(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•13 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•13 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•13 years ago
|
||
It was caused with corrupted chromeappsstore.sqlite.
I think that the corruption is triggered by landing of Bug 592431.
Assignee | ||
Comment 9•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Assignee | ||
Comment 10•13 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•13 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•13 years ago
|
Attachment #556706 -
Attachment is obsolete: true
Assignee | ||
Comment 12•13 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.
Comment 13•13 years ago
|
||
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•13 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?
Comment 15•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
Beta. (7). Possibly also in Aurora or nightly but we don't have as much data there.
Assignee | ||
Comment 19•13 years ago
|
||
Confirmed.
The problem returns.
Delleting chromeappsstore.sqlite helps.
Status: VERIFIED → REOPENED
Resolution: WORKSFORME → ---
Assignee | ||
Comment 20•13 years ago
|
||
s/Delleting/Deleting/
Assignee | ||
Comment 21•13 years ago
|
||
Comment 22•13 years ago
|
||
Reproducing this with: Mozilla/5.0 (X11; Linux i686; rv:7.0.1) Gecko/20100101 Firefox/7.0.1
:-(
Assignee | ||
Updated•13 years ago
|
Status: REOPENED → NEW
Comment 23•13 years ago
|
||
The exactly same problem is being experienced.
Comment 24•13 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•13 years ago
|
||
This problem happens if iframe is included in snippets.
I think that pagehide event from the iframe should be ignored.
Comment 27•13 years ago
|
||
Looks like we pushed another snippet this week that broke this.
Comment 28•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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)
Comment 32•13 years ago
|
||
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•13 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.
Comment 34•13 years ago
|
||
If you need someone from Product QA, please loop in Ioana Budnar (our QA point of contact for Session Store).
Comment 35•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
Component: Session Restore → General
QA Contact: session.restore → general
Whiteboard: [about-home]
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 40•13 years ago
|
||
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•13 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+
Comment 42•13 years ago
|
||
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•13 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•13 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.
Comment 45•13 years ago
|
||
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•13 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. :-/
Comment 48•13 years ago
|
||
Attachment #573230 -
Attachment is obsolete: true
Attachment #617932 -
Flags: review?(gavin.sharp)
Comment 49•13 years ago
|
||
Typo.
Attachment #617932 -
Attachment is obsolete: true
Attachment #617932 -
Flags: review?(gavin.sharp)
Attachment #617933 -
Flags: review?(gavin.sharp)
Comment 50•13 years ago
|
||
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 51•13 years ago
|
||
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•13 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•13 years ago
|
||
Thanks, everyone! How long until this could land in any Firefox channel?
Comment 54•13 years ago
|
||
Backed out for m-oth failures:
https://hg.mozilla.org/integration/fx-team/rev/ab8c08dfe9c4
Whiteboard: [about-home][fixed-in-fx-team] → [about-home]
Comment 55•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Comment 56•13 years ago
|
||
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [about-home] → [about-home][backed-out]
Comment 57•13 years ago
|
||
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 58•13 years ago
|
||
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•13 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
Comment 60•13 years ago
|
||
(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•13 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?
Updated•13 years ago
|
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
Updated•13 years ago
|
Assignee: nobody → fryn
Component: File Handling → General
QA Contact: file.handling → general
Target Milestone: Firefox 14 → ---
Comment 62•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
Attachment #632923 -
Flags: review?(gavin.sharp)
Updated•13 years ago
|
Attachment #632922 -
Flags: review?(gavin.sharp) → review+
Comment 65•13 years ago
|
||
Can we write a test that covers this case?
Updated•13 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•13 years ago
|
||
Status: NEW → ASSIGNED
Updated•13 years ago
|
Attachment #632922 -
Attachment description: patch v5: part 1 → patch v5
Attachment #632922 -
Attachment filename: abouthomefixrestore1 → abouthomefixrestore
Updated•13 years ago
|
Attachment #632923 -
Attachment filename: abouthomefixrestore → abouthomefixrestorepart2
Attachment #632923 -
Attachment is obsolete: true
Attachment #632923 -
Flags: review?(gavin.sharp)
Comment 67•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Comment 68•13 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.
Comment 69•13 years ago
|
||
(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 70•13 years ago
|
||
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+
Updated•13 years ago
|
status-firefox15:
--- → affected
Comment 71•13 years ago
|
||
(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•13 years ago
|
||
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•13 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•13 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•13 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 76•13 years ago
|
||
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?
Comment 77•13 years ago
|
||
Chris: assuming we take this on beta, the fix will be included with Firefox 14, released on July 17.
Comment 78•13 years ago
|
||
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+
Comment 79•13 years ago
|
||
Frank, can you take care of the aurora/beta landings?
Updated•13 years ago
|
status-firefox14:
--- → affected
tracking-firefox14:
--- → +
Comment 80•12 years ago
|
||
Marking this verified fixed using the test in comment 74 against Firefox 16.0a1 2012-06-25.
Comment 81•12 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
Comment 82•12 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•