Closed
Bug 789037
Opened 12 years ago
Closed 11 years ago
Make sure that bug 776928 has no major addon-compat impact
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
WORKSFORME
Tracking | Status | |
---|---|---|
firefox16 | --- | unaffected |
firefox17 | --- | unaffected |
firefox18 | + | fixed |
firefox19 | + | verified |
firefox20 | - | --- |
People
(Reporter: dao, Unassigned)
References
Details
(Keywords: addon-compat, regression)
Attachments
(1 file)
3.36 KB,
patch
|
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
STR: - set browser.ctrlTab.previews to true - with less than 7 pinned tabs and at least two normal tab open, restart the browser, saving your session - hit Ctrl+Tab
Reporter | ||
Comment 1•12 years ago
|
||
Bug 789010 will avoid this particular bug, but the underlying regression is still concerning in terms of potentially breaking other code, including add-ons.
Depends on: 789010
Reporter | ||
Updated•12 years ago
|
Severity: normal → major
Updated•12 years ago
|
Comment 2•12 years ago
|
||
Assigning to Tim given bug 776928 appears to be the regressing issue. Tim - can you speak to Dao's concerns here w/r/t add-on compat? We'll verify that comment 0 is fixed in bug 789010.
Assignee: nobody → ttaubert
status-firefox16:
--- → unaffected
status-firefox17:
--- → unaffected
status-firefox18:
--- → affected
status-firefox19:
--- → affected
Summary: Ctrl+Tab fails when encountering a tab that isn't restored yet → Make sure that bug 776928 has no major addon-compat impact
Comment 3•12 years ago
|
||
It's a small change but it definitely has potential to break things. So far there haven't been any further bug reports related to it but that doesn't mean it couldn't break a couple of add-ons.
Reporter | ||
Comment 4•12 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #3) > It's a small change but it definitely has potential to break things. So far > there haven't been any further bug reports related to it Bug 810584 was filed a few days ago.
Comment 5•12 years ago
|
||
Oh, I didn't see this one, thanks. So we have two options: we can back this change out if we expect more breakage. This should be really easy because the patch is tiny. The other option is to reach out to the add-on author and make sure they fix it to work with Fx 18.
Comment 6•12 years ago
|
||
What kind of breakage should we expect? The opening comment seems to indicate that the DOM works correctly for unloaded tabs, while the style properties don't. Is this correct?
Reporter | ||
Comment 7•12 years ago
|
||
(In reply to Jorge Villalobos [:jorgev] from comment #6) > What kind of breakage should we expect? All kinds of breakage whenever something attempts to access the content of background tabs.
Comment 8•12 years ago
|
||
This sounds like something that can break many add-ons in various ways. I'd like to have more time to let developers know about it so that they can adapt their add-ons before they break for everyone. What's the easiest way to detect a tab hasn't loaded yet?
Comment 9•12 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #7) > All kinds of breakage whenever something attempts to access the content of > background tabs. ... the content of not-yet-restored tabs. (In reply to Jorge Villalobos [:jorgev] from comment #8) > This sounds like something that can break many add-ons in various ways. I'd > like to have more time to let developers know about it so that they can > adapt their add-ons before they break for everyone. Understandable. The patch would be really easy to back out or disable. We would then a gain a couple of MB (KB?) per unrestored tab but that's probably not as bad as breaking add-ons. > What's the easiest way to detect a tab hasn't loaded yet? if (tab.getAttribute("pending")) { // tab hasn't loaded yet... } if (tab.linkedBrowser.getAttribute("pending")) { // tab hasn't loaded yet... }
Comment 10•12 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #9) > Understandable. The patch would be really easy to back out or disable. We > would then a gain a couple of MB (KB?) per unrestored tab but that's > probably not as bad as breaking add-ons. If we push it back to 19, that should be sufficient time to give developers to test and fix their add-ons.
Reporter | ||
Comment 11•12 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #9) > (In reply to Dão Gottwald [:dao] from comment #7) > > All kinds of breakage whenever something attempts to access the content of > > background tabs. > > ... the content of not-yet-restored tabs. Which is an distinction that affected code wouldn't make, so I didn't mention it. The memory saved by bug 776928 is pretty small. I think we should consider WONTFIXing it.
Comment 12•12 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #11) > The memory saved by bug 776928 is pretty small. I think we should consider > WONTFIXing it. I wouldn't object. Who can make this call? Should we at least notify MemShrink people?
Comment 13•12 years ago
|
||
Justin/Nick, I'd like to make a call here about backing out bug 776928 due to its add-on impact (see also: bug 810584). I don't see much discussion in that bug about the magnitude of the improvement expected - do you guys have any relevant data, or barring that, opinions? :)
Comment 14•12 years ago
|
||
> I don't see much discussion in that bug about the magnitude of the improvement expected -
> do you guys have any relevant data, or barring that, opinions? :)
Is the idea that we'd never be able to make this change, or that we'd be able to do it after notifying add-on devs?
I recall that the memory saved was on the order of a few hundred KB per bg tab. For "regular" users this isn't particularly significant, but for power users who tend to have hundreds of unloaded tabs, it's quite significant. Power users are a small but disproportionately-influential group.
Updated•12 years ago
|
Assignee: ttaubert → gavin.sharp
Comment 15•12 years ago
|
||
It saves 0.17MB per not-yet-restored tab on 64-bit platforms, and slightly less on 32-bit platforms. I'll be sad if we have to back it out permanently; some users really do have hundreds of these not-yet-loaded tabs.
Comment 16•12 years ago
|
||
Then I suggest we go back to the plan of pushing this back to 19.
Comment 17•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #14) > Is the idea that we'd never be able to make this change, or that we'd be > able to do it after notifying add-on devs? I don't think we're at the point where we've determined it's impossible. Evangelism for a change of this nature can be rather difficult (it's not trivial to create e.g. a warning in the console anytime you do something that might break), but we can give it a try.
Comment 18•12 years ago
|
||
> I don't think we're at the point where we've determined it's impossible. Evangelism for a
> change of this nature can be rather difficult (it's not trivial to create e.g. a warning
> in the console anytime you do something that might break), but we can give it a try.
I guess the distinction I should have tried to draw is between notifying add-on devs and waiting for them to fix their stuff. It sounds like you're suggesting the latter approach.
It seems unlikely that we'll be able to fix all add-ons via evangelism, if our experience trying to get add-on leaks fixed is any guide. Even with the large hammer of downgrading leaky add-ons to preliminary status, we weren't able to fix all the add-on leaks until we implemented a fix in Gecko. I doubt anyone is going to troll through our add-ons to find ones which are incompatible with this change; it's likely not worth it.
Before rapid release, we were willing to break add-ons to move the platform forward. I totally understand if we want to give add-on devs time to react to our changes in the rapid-release world, but that's not the same as predicating our ability to make changes on the add-on devs fixing their code.
Comment 19•12 years ago
|
||
[Approval Request Comment] Bug caused by (feature/regressing bug #): 776928 User impact if declined: addon compat issues Testing completed (on m-c, etc.): none, straight backout Risk to taking this patch (and alternatives if risky): none String or UUID changes made by this patch: none
Attachment #685918 -
Flags: approval-mozilla-beta?
Attachment #685918 -
Flags: approval-mozilla-aurora?
Comment 20•12 years ago
|
||
I wasn't suggesting we just wait until we have 100% of addons updated so they don't break as a result of this change; obviously that's impossible. On the other hand, we've made no effort to evangelize this change ahead of time, or to investigate ways to increase add-on developer awareness of the issue, and I think we need to before we let this change ship. Any help you or anyone else could offer in that area would be most welcome!
Updated•12 years ago
|
tracking-firefox20:
--- → +
Updated•12 years ago
|
Attachment #685918 -
Flags: approval-mozilla-beta?
Attachment #685918 -
Flags: approval-mozilla-beta+
Attachment #685918 -
Flags: approval-mozilla-aurora?
Attachment #685918 -
Flags: approval-mozilla-aurora+
Comment 21•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/add4aedc517f https://hg.mozilla.org/releases/mozilla-beta/rev/ff4b7cf32d5e
Comment 22•11 years ago
|
||
The current plan is to ship this change in Firefox 20, right?
Comment 23•11 years ago
|
||
That's the current default state, yeah. I haven't had a chance to investigate this further.
Assignee: gavin.sharp → nobody
Comment 24•11 years ago
|
||
Verified bug 776928 is disabled on FF 19b6.
Comment 25•11 years ago
|
||
We'll try to get an add-ons post/developer docs for this in bug 776928. I'm not sure that there's much else actionable here. I haven't seen any additional reports, but this is only going to hit beta again next week. We'll need to keep an eye out for additional add-on bustage feedback.
Comment 26•11 years ago
|
||
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #25) > We'll try to get an add-ons post/developer docs for this in bug 776928. I'm > not sure that there's much else actionable here. I haven't seen any > additional reports, but this is only going to hit beta again next week. > We'll need to keep an eye out for additional add-on bustage feedback. Thanks for following up in bug 810584 (LastTab) as well. Since we have a plan for documenting this (https://bugzilla.mozilla.org/show_bug.cgi?id=776928#c27) and but 810584 isn't critical, no need to continue tracking.
Comment 27•11 years ago
|
||
Doesn't seem like there's anything useful to track here.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•