The custom content container should not have a frame when it's not used

RESOLVED FIXED in mozilla38

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

({perf})

Trunk
mozilla38
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
STEPS TO REPRODUCE
1. firefox -layoutdebug about:blank
2. Dump -> Frames

ACTUAL RESULTS
The ViewportFrame has a fixed pos child frame.  This is for the
"custom content container" added in bug 1020244.

I find this rather annoying when trying to debug frame tree issues.
It's also a memory usage / performance issue.

I think we should create this frame only when the custom content container
actually has any children (i.e. when someone opens DevTools), so a frame
isn't needed normally.
(Assignee)

Updated

4 years ago
Assignee: nobody → mats
(Assignee)

Comment 1

4 years ago
I should also mention that devtools actually fails to remove the anon content
it added, so if you open/close DevTools on the same page a few times, the
number of children in the document's custom content container grows.
Blocks: 985597
(Assignee)

Comment 2

4 years ago
Created attachment 8542890 [details] [diff] [review]
part 1 - Store the document where we added anonomous content so we can call removeAnonymousContent() on it later without throwing.

This fixes the problem described in comment 1.
Attachment #8542890 - Flags: review?(pbrosset)
(Assignee)

Comment 3

4 years ago
Created attachment 8542891 [details] [diff] [review]
part 2 - Don't create a frame for the custom content container when it has no children.

Create/destroy the frame on demand by toggling @hidden.
Attachment #8542891 - Flags: review?(roc)
(Assignee)

Comment 4

4 years ago
Created attachment 8542892 [details] [diff] [review]
part 3 - Use GetParentOrPlaceholderFor (not GetParent) so that reframing anonymous content frames works also for fixed pos frames.

https://tbpl.mozilla.org/?tree=Try&rev=01359209c378
Attachment #8542892 - Flags: review?(roc)
Comment on attachment 8542890 [details] [diff] [review]
part 1 - Store the document where we added anonomous content so we can call removeAnonymousContent() on it later without throwing.

Review of attachment 8542890 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Good catch. Thanks!
Attachment #8542890 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 8

4 years ago
Sorry about that, Ryan.  It's definitely surprising that this change caused a failure
in Opt builds only.  Investigating...
(Assignee)

Comment 9

4 years ago
OK, so part 1 by itself fails with the same error:
https://tbpl.mozilla.org/?tree=Try&rev=0aff472a11e3

It looks like an existing bug in RemoveAnonymousContent:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?rev=cfccc5448963#5297
(the return type of Length() is size_t so when it is zero ... oops)
(Assignee)

Comment 10

4 years ago
Created attachment 8547038 [details] [diff] [review]
part 0 - Iterate forward instead, to avoid depending on undefined integer behavior.

None of these loops needs to iterate backwards.  Only the first modifies
the array and it explicitly breaks out of the loop after that.

This patch doesn't fix the failed test unfortunately, but is seems worth
taking anyway to simplify the code.
Attachment #8547038 - Flags: review?(roc)
(Assignee)

Comment 11

4 years ago
With part 1 applied (so we actually get a call to nsIDocument::RemoveAnonymousContent)
the test browser/devtools/webconsole/test/browser_bug_871156_ctrlw_close_tab.js
times out.  But only in Opt builds.  I can't reproduce that locally unfortunately,
neither in Debug nor Opt builds (on Linux64 and OSX).  I'm guessing it's timing
dependent.  So far, I've concluded that it is the RemoveElementAt(i) here:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?rev=00996e98fa62#5313
that makes the test time out.  Putting in a 'return;' after the RemoveElementAt
call fails consistently: https://tbpl.mozilla.org/?tree=Try&rev=0be7353043b2
Putting a return before the RemoveElementAt call makes the tests pass (but then
RemoveAnonymousContent isn't really doing anything of course).

All this smells kind of fishy to me.  I don't see why removing an entry from an
array should cause a test failure like that.  And it's not the removing as such;
dropping of the ref count is enough -- I tried two variations of that, one that
nulled out the AnonymousContent* in the array entry, and one version that nulled
out the mContentNode inside the AnonymousContent itself - both fails in the
same way.

Can anyone else reproduce it locally (in an Opt build, with part 1 applied)?
(I'm guessing a slow-ish machine has a better chance making the test fail)
The command is:
./mach mochitest-devtools browser/devtools/webconsole/test/
(Assignee)

Comment 12

4 years ago
Created attachment 8547049 [details]
Log snippet for posterity

I think the first "this.docShell is null" error is a red herring -
it occurs also when the test succeeds, so all is well up to the
"promise.all resolved. start testing the Browser Console" message.
http://hg.mozilla.org/mozilla-central/annotate/84880f27bab3/browser/devtools/webconsole/test/browser_bug_871156_ctrlw_close_tab.js
The "error occurred while processing 'getCachedMessages:..." that
follows probably has something to do with the timeout - I don't
see that error in a test run that succeeds.
(Assignee)

Comment 13

4 years ago
Created attachment 8547095 [details]
Log snippet #2

FWIW, when I comment out the latter part of browser_bug_871156_ctrlw_close_tab.js
the failure occurs in the second test after it instead: browser_console.js
https://tbpl.mozilla.org/?tree=Try&rev=fdea70e13ed2

The error messages appears to be the same as in the unmodified
browser_bug_871156_ctrlw_close_tab.js case so I'm starting to suspect
that this is an existing bug in the devtools code.  It seems that by
not releasing the anonymous content extends the lifetime of something,
which hides that bug.
(Assignee)

Comment 14

4 years ago
Landed part 0:
https://hg.mozilla.org/integration/mozilla-inbound/rev/324793f2a92b
Keywords: leave-open
(Assignee)

Comment 15

4 years ago
I'm stuck here on the devtools test failures which I don't know how to fix.
Patrick, is that something you can help on?  or know someone on the devtools
team who can?
Flags: needinfo?(pbrosset)
(In reply to Mats Palmgren (:mats) from comment #15)
> I'm stuck here on the devtools test failures which I don't know how to fix.
> Patrick, is that something you can help on?  or know someone on the devtools
> team who can?
Sorry for the delay getting involved here. I'll start taking a look at the devtools test failures today.
Flags: needinfo?(pbrosset)
I can't reproduce this issue either (after applying the part 1 patch, OPT build, OSX).

The weird thing is that during the webconsole tests suite execution, there's no call to removeAnonymousContent before the browser_console_native_getters.js test, which runs a lot later than browser_bug_871156_ctrlw_close_tab.js, so I don't see how a change in this function could impact this test.

Also, worth noting the test only fails on Linux, Opt, non-e10s, it seems to pass fine on other platforms, and when e10s is ON.

I agree that the "getCachedMessages" error message is related to the timeout, but it's odd because it's a message for an error that occurs on the actor side, when retrieving messages for the console, so it has nothing to do with inserting/removing anonymous content into the canvas frame, this is done at a later stage, when console messages are sent back to the UI and contain DOM nodes.

The stacktrace shows that the error occurs in DevToolsUtils.js:301, when calling |Cu.getGlobalForObject(aObj)|, which is something called while processing the |getCachedMessages| request.
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #18)
> The stacktrace shows that the error occurs in DevToolsUtils.js:301, when
> calling |Cu.getGlobalForObject(aObj)|, which is something called while
> processing the |getCachedMessages| request.
Also worth noting that |Cu.getGlobalForObject(aObj)| isn't called during browser_bug_871156_ctrlw_close_tab.js, so the error is probably from an earlier test, being reported late.
Hmmm, I pushed to try (dt,linux,opt,non-e10s) with part 1 applied and a debug patch with various logs here and there for me to understand the problem better and it came back green :|
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a824ead7249c
What am I missing?
(Assignee)

Comment 21

4 years ago
Created attachment 8550807 [details] [diff] [review]
part 1 - Store the document where we added anonomous content so we can call removeAnonymousContent() on it later without throwing.

Hmm, I don't know why that makes it green, but I'm totally going to
exploit that as a wallpaper so I can land my changes.

I'll file a follow-up bug for the devtools test failures without the
Cu.getGlobalForObject thing.
Attachment #8542890 - Attachment is obsolete: true
Attachment #8550807 - Flags: review?(pbrosset)
Comment on attachment 8550807 [details] [diff] [review]
part 1 - Store the document where we added anonomous content so we can call removeAnonymousContent() on it later without throwing.

Review of attachment 8550807 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/highlighter.js
@@ +421,5 @@
>  function CanvasFrameAnonymousContentHelper(tabActor, nodeBuilder) {
>    this.tabActor = tabActor;
>    this.nodeBuilder = nodeBuilder;
> +  this.anonymousContentDocument = this.tabActor.window.document;
> +  this.anonymousContentGlobal = Cu.getGlobalForObject(this.anonymousContentDocument);

So if I understand correctly, you're saying what makes the test green is adding this line in? Even though this.anonymousContentGlobal isn't used anywhere?
If yes, then definitely a follow-up bug is needed, but please also add a comment before this line explaining why this is here, and mentioning the bug number.
Attachment #8550807 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 24

4 years ago
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #23)
> So if I understand correctly, you're saying what makes the test green is
> adding this line in? Even though this.anonymousContentGlobal isn't used
> anywhere?

Yes, just calling getGlobalForObject and keeping the reference seems to be enough.

> If yes, then definitely a follow-up bug is needed, but please also add a
> comment before this line explaining why this is here, and mentioning the bug
> number.

Done, bug 1123362.

Comment 27

4 years ago
Note that https://hg.mozilla.org/mozilla-central/rev/700835de77fe adds ability to style anonymous canvas frame.

I used the next workaround for ff37:

> /** 
>  * Manually trigger reflow for document because we can't do that for anonymouse element
>  * https://developer.mozilla.org/en-US/docs/Tools/Web_Console#synchronous-and-asynchronous-reflows
>  * this is not necessary in ff38 because of https://hg.mozilla.org/mozilla-central/rev/700835de77fe
>  */
> function windowContentReflow(win) {
>   let contentWin = win.content.window,
>       oldDisplay = contentWin.document.documentElement.style.display;
> 
>   contentWin.document.documentElement.style.display = 'none';
>   contentWin.getComputedStyle(contentWin.document.documentElement).display; // force reflow
>   contentWin.document.documentElement.style.display = oldDisplay;
>   contentWin.getComputedStyle(contentWin.document.documentElement).display;
> }

You can see the working example here https://github.com/firebug/pixel-perfect/pull/66/files
You need to log in before you can comment on or make changes to this bug.