sync-illustration.svg is loaded but not shown at browser startup

RESOLVED FIXED in Firefox 57

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: johannh, Assigned: eoger)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 57
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [reserve-photon-performance])

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
According to our new browser_startup_image.js test, sync-illustration.svg is loaded during browser startup but not shown. For startup performance and memory usage, we should avoid loading images that aren't going to be displayed.
(Reporter)

Comment 1

2 years ago
Mark, as the author of the patches in bug 1201331, can you please help resolve this issue?
Flags: needinfo?(markh)
Ed, do you mind having a look at this?
Flags: needinfo?(markh) → needinfo?(eoger)
(Assignee)

Comment 3

2 years ago
I think this is because of http://searchfox.org/mozilla-central/source/browser/components/customizableui/content/panelUI.inc.xul#187, which is weird because it is under #PanelUI-popup which is hidden by default.
Flags: needinfo?(eoger)
There's probably a missing hidden="true" somewhere, but I don't see where. Is there a reason why this is using html:img rather than xul image tags? (I'm not aware of this being a problem, but it seems to be a difference compared to the other images here, so I wonder if it has an impact.)
(Assignee)

Comment 5

2 years ago
Will investigate.
Flags: needinfo?(eoger)
(Assignee)

Comment 6

2 years ago
I can't reproduce, instead the tests are failing for me: "sync-illustration.svg should have been loaded"
Flags: needinfo?(eoger)
(Reporter)

Comment 7

2 years ago
(In reply to Edouard Oger [:eoger] from comment #6)
> I can't reproduce, instead the tests are failing for me:
> "sync-illustration.svg should have been loaded"

Are you testing on a debug build? It doesn't work otherwise.
(Assignee)

Updated

2 years ago
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Priority: -- → P1

Comment 9

2 years ago
mozreview-review
Comment on attachment 8897962 [details]
Bug 1380377 part 2 - Make sure sync-illustration.svg doesn't get loaded on startup.

https://reviewboard.mozilla.org/r/169270/#review174702

thanks!
Attachment #8897962 - Flags: review?(markh) → review+

Comment 10

2 years ago
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b0f76b216a19
Make sure sync-illustration.svg doesn't get loaded on startup. r=markh
Whiteboard: [reserve-photon-performance]

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b0f76b216a19
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Iteration: --- → 57.2 - Aug 29
Flags: qe-verify?
Flags: qe-verify? → qe-verify-
(Reporter)

Comment 12

2 years ago
Awesome, thanks for fixing this!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Iteration: 57.2 - Aug 29 → ---
Target Milestone: Firefox 57 → ---
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

2 years ago
Florian does the approach I took makes sense?
I wonder if we might miss the first MozAfterPaint even since we’re registering the event handler *after* we open the new window.

Comment 17

2 years ago
mozreview-review
Comment on attachment 8902419 [details]
Bug 1380377 part 1 - Calculate FX_NEW_WINDOW_MS using MozAfterPaint.

https://reviewboard.mozilla.org/r/173998/#review179570

(In reply to Edouard Oger [:eoger] from comment #16)
> Florian does the approach I took makes sense?

Yes (r- because I have some trivial comments below)

> I wonder if we might miss the first MozAfterPaint even since we’re
> registering the event handler *after* we open the new window.

I don't think so.

::: browser/base/content/browser.js:4186
(Diff revision 1)
>    var telemetryObj = {};
>    TelemetryStopwatch.start("FX_NEW_WINDOW_MS", telemetryObj);
>  
> -  function newDocumentShown(doc, topic, data) {
> -    if (topic == "document-shown" &&
> -        doc != document &&
> +  function mozAfterPaint(e) {
> +    if (e.target.document != document &&
> +        e.target.document.defaultView == win) {

I don't think we need these tests anymore with an event listener on the window instead of a global observer notification.

::: browser/base/content/browser.js:4187
(Diff revision 1)
>    TelemetryStopwatch.start("FX_NEW_WINDOW_MS", telemetryObj);
>  
> -  function newDocumentShown(doc, topic, data) {
> -    if (topic == "document-shown" &&
> -        doc != document &&
> -        doc.defaultView == win) {
> +  function mozAfterPaint(e) {
> +    if (e.target.document != document &&
> +        e.target.document.defaultView == win) {
> +      win.removeEventListener("MozAfterPaint", mozAfterPaint);

Could we use the {once: true} option of addEventListener instead?

::: browser/base/content/browser.js:4200
(Diff revision 1)
> -      Services.obs.removeObserver(newDocumentShown, "document-shown");
> +      win.removeEventListener("MozAfterPaint", mozAfterPaint);
>        Services.obs.removeObserver(windowClosed, "domwindowclosed");
>      }
>    }
>  
> -  // Make sure to remove the 'document-shown' observer in case the window
> +  // Make sure to remove the 'MozAfterPaint' observer in case the window

It's not an observer, but an event listener.

And I doubt that would still leak, but I'm not opposed to keeping the windowClosed observer if we are not sure.
Attachment #8902419 - Flags: review?(florian) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

2 years ago
mozreview-review
Comment on attachment 8902419 [details]
Bug 1380377 part 1 - Calculate FX_NEW_WINDOW_MS using MozAfterPaint.

https://reviewboard.mozilla.org/r/173998/#review179944

Looks good to me, thanks!

Before landing, please verify:
- on try that this doesn't leak (ie. run all mochitests on debug builds on Windows/Linux/Mac)
- locally that the measured times for FX_NEW_WINDOW_MS shown on about:telemetry are similar before/after applying the sync-illustration.svg changes.
Attachment #8902419 - Flags: review?(florian) → review+
(Assignee)

Comment 21

2 years ago
All good, landing!

Comment 22

2 years ago
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9d56bf3a749d
part 1 - Calculate FX_NEW_WINDOW_MS using MozAfterPaint. r=florian
https://hg.mozilla.org/integration/autoland/rev/77983a8e6ce9
part 2 - Make sure sync-illustration.svg doesn't get loaded on startup. r=markh
https://hg.mozilla.org/mozilla-central/rev/9d56bf3a749d
https://hg.mozilla.org/mozilla-central/rev/77983a8e6ce9
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Iteration: --- → 57.3 - Sep 19
You need to log in before you can comment on or make changes to this bug.