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)

defect
Not set
major

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)

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
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
Severity: normal → major
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
Summary: Ctrl+Tab fails when encountering a tab that isn't restored yet → Make sure that bug 776928 has no major addon-compat impact
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.
(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.
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.
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?
(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.
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?
(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...
}
(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.
(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.
(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?
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? :)
> 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.
Assignee: ttaubert → gavin.sharp
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.
Then I suggest we go back to the plan of pushing this back to 19.
(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.
> 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.
[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?
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!
Attachment #685918 - Flags: approval-mozilla-beta?
Attachment #685918 - Flags: approval-mozilla-beta+
Attachment #685918 - Flags: approval-mozilla-aurora?
Attachment #685918 - Flags: approval-mozilla-aurora+
The current plan is to ship this change in Firefox 20, right?
That's the current default state, yeah. I haven't had a chance to investigate this further.
Assignee: gavin.sharp → nobody
Verified bug 776928 is disabled on FF 19b6.
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.
(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.
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.