Closed Bug 1362378 Opened 7 years ago Closed 7 years ago

[e10s] Web content fails to load after reloading all tabs

Categories

(Firefox :: Tabbed Browser, defect)

Product:
Firefox
Component:
Tabbed Browser
Version:
55 Branch
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox-esr45 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 - verified
firefox56 + verified

People

(Reporter: emilghitta, Assigned: kmag)

References

Details

(Keywords: dataloss, regression)

Attachments

(2 files)

Reload Tabs.gif
5.43 MB, image/gif
Details
Bug 1362378: Restore rather than reload pending background tabs when reloading all tabs.
59 bytes, text/x-review-board-request
dao
: review-
Details
Attached image Reload Tabs.gif — 
[Affected versions]:
Firefox 55.0a1 (Build Id:20170505030252)

[Unaffected versions]:
Firefox 54.0b4 (Build Id:20170501133120)
Firefox 53.0 (Build Id:20170413192749)

[Affected platforms]:
Windows 10 64bit
Mac 10.11.6
Ubuntu 16.04 64bit


[Steps to reproduce]:
1.Launch Firefox with a clean profile.
2.Enable e10s (if it is not already enabled).
3.Access the https://www.youtube.com/ webpage.
4.Open several videos in new tabs.
5.Restart Firefox.
6.Reload all Tabs (without focusing them).
7.Focus the opened tabs.
8.Observe that the content is not displayed.

[Expected result]:
Content is displayed after reloading all tabs.

[Actual result]:
No content is displayed.

[Regression range]:
This is a regression:

Last good revision: f77767c9f2bcc78addef282be397ac869cb3dca8
First bad revision: fb4fca2b150a3715192e60e4dde593052fb563e2

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f77767c9f2bcc78addef282be397ac869cb3dca8&tochange=fb4fca2b150a3715192e60e4dde593052fb563e2

It seems that bug 1353060 may have caused this issue.

[Additional Notes]
Please note that the content is not displayed even if refreshing the tab.
It seems that this issue is not reproducible while having e10s disabled.
For further information regarding this issue, please observe the attached video.
Hi Kris!

Can you please have a look?

Thanks !
Flags: needinfo?(kmaglione+bmo)
Blocks: 1353060
[Tracking Requested - why for this release]: Regression
Severity: major → critical
status-firefox-esr45: --- → unaffected
tracking-firefox55: --- → ?
Keywords: dataloss
Tracking as new regression in 55.
tracking-firefox55: ? → +
Blocks: 1366103
I think it is time to backing the offending bug(bug 1353060) out
Severity: critical → blocker
(In reply to Alice0775 White from comment #5)
> I think it is time to backing the offending bug(bug 1353060) out

It is definitely not. This only happens when extensions do something they should not be doing (reload a tab that hasn't been initialized yet). If it's not fixed before the merge, we can consider backing out.
Severity: blocker → critical
Flags: needinfo?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #6)
> This only happens when extensions do something they should not be doing .

Extensions? Does it mean add-ons?

I can reproduce the data loss without any add-ons.
I mean extensions.

Hm. I didn't realize we had a Reload All Tabs feature with not extensions installed. That almost certainly should not be touching uninitialized tabs.
Comment on attachment 8872194 [details]
Bug 1362378: Restore rather than reload pending background tabs when reloading all tabs.

https://reviewboard.mozilla.org/r/143648/#review150670

::: browser/base/content/tabbrowser.xml:3351
(Diff revision 1)
> +        <parameter name="aKeepTemporaryPermissions"/>
>          <body>
>            <![CDATA[
> -            let browser = this.getBrowserForTab(aTab);
> +            if (!aKeepTemporaryPermissions) {
> -            // Reset temporary permissions on the current tab. This is done here
> +              // Reset temporary permissions on the current tab. This is done here
> -            // because we only want to reset permissions on user reload.
> +              // because we only want to reset permissions on user reload.

I'm not a fan of adding this boolean parameter. It doesn't help that the comment says "we only want to reset permissions on user reload" and "reload all tabs" is in fact a user action. Pretty obscure stuff.
Attachment #8872194 - Flags: review?(dao+bmo) → review-
Kris, how important is this bug? Something we wanna land in 55, or uplift in early beta? Or can it wait for 56?
Flags: needinfo?(kmaglione+bmo)
The patches for bug 1362462 have fixed the immediate regression, but our behavior in this circumstance is still pretty flaky, so I think we should fix this in 55 regardless. I'll try to have an updated patch later today.
Flags: needinfo?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #12)
> I'll try to have an updated patch later today.

Did you ever get around to that patch?
Flags: needinfo?(kmaglione+bmo)
Moving to Firefox:TabbedBrowser which seems a better fit for this issue.
Component: Layout → Tabbed Browser
Product: Core → Firefox
Dão, not sure if this is something you can help with (as it is now in "your" component or if Kris is still planning on a patch for 56/55.
tracking-firefox56: ? → +
Flags: needinfo?(dao+bmo)
Hi everyone ! 

It seems that I can't reproduce this issue anymore.

This issue was fixed by another patch:

First good revision: 63cc19a2300f356abf1fa03884a9ac3d4792ef57
Last bad revision: 72a455b224e63335c4cf6cce7588d2b9c9de7a5f

Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=72a455b224e63335c4cf6cce7588d2b9c9de7a5f&tochange=63cc19a2300f356abf1fa03884a9ac3d4792ef57

Looks like the following bug has the  changes which introduced the fix:
https://bugzilla.mozilla.org/show_bug.cgi?id=1362462
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #15)
> Dão, not sure if this is something you can help with (as it is now in "your"
> component or if Kris is still planning on a patch for 56/55.

I don't know what Kris is planning, his needinfo request is still pending. But based on comment 12, it seems that we don't need to track this for 55 anymore.
Flags: needinfo?(dao+bmo)
This may be a duplicate of bug 1362462. I'll continue discussion there.
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fix-optional
status-firefox56: affected → fixed
tracking-firefox55: + → -
Resolution: --- → FIXED
Assignee: nobody → kmaglione+bmo
status-firefox55: fix-optional → fixed
Target Milestone: --- → Firefox 55
This issue is verified fixed using Firefox 56.0.2 (BuildId:20171024165158) and Firefox 55.0.3 (BuildId:20170824053622) on Windows 10 64bit, macOS 10.11.6 and Ubuntu 16.04 64bit.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
status-firefox56: fixed → verified
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: