Closed Bug 1500543 Opened Last year Closed Last year

Extremely spammy warning |WARNING: NS_ENSURE_TRUE(browserChrome) failed: file [snip]/docshell/base/nsDocShell.cpp, line 12726|


(Core :: Document Navigation, defect, P2)




Tracking Status
firefox-esr60 --- unaffected
firefox63 --- unaffected
firefox64 --- fixed
firefox65 --- fixed


(Reporter: jorgk, Assigned: ehsan)



(Keywords: regression)


(3 files)

In Thunderbird's MozMill run here

we see
WARNING: NS_ENSURE_TRUE(browserChrome) failed: file /builds/worker/workspace/build/src/docshell/base/nsDocShell.cpp, line 12726

4792 times :-(

It comes from here:
(different line number, file must have changed meanwhile).

That code has been there since 2015, and I don't know what triggers it now, but it is rather undesired to see all that noise in the debug console.

I haven't tried in FF, is in not present there?

Sorry Boris, I don't know better how to attract some attention here.
Flags: needinfo?(bzbarsky)
It's pretty odd to not have an nsIWebBrowserChrome there....   What sort of object is mTreeOwner?
Flags: needinfo?(bzbarsky)
Attached file mTreeOwner-null.txt
I see this running a debug version locally all the time.

I've added some debug:
  if (!browserChrome) {
    printf("=== %p\n", mTreeOwner);
and that shows that mTreeOwner==nullptr.

Here's a stack at that point. Well, that's one case I caught, there may be more. I got this one by opening the subscribe dialogue for an IMAP server. Opening a message in a stand-alone windows gives the same, followed by another one which I'll attach as well.
Attached file mTreeOwner-null2.txt
Priority: -- → P2
Thanks.  So this is the initial docshell creation in both cases, before we have had a chance to set a treeowner.

I'm pretty surprised that nsDocShell::SetDocLoaderParent is calling EnsureScriptEnvironment.  I guess that's coming from this code there;

  nsCOMPtr<nsPIDOMWindowOuter> window = GetWindow();
  if (window) {
    auto* win = nsGlobalWindowOuter::Cast(window);

which was added in bug 1498720.  Ehsan, should we restrict this block to the case when aParent is null (corresponding to removal)?  Or restrict it to cases when we already have an mScriptGlobal or something?  Creating a window here if we don't have one already seems odd.
Blocks: 1498720
Flags: needinfo?(ehsan)
Maybe we should just add GetExistingWindow() or something.
Oh, yes, my bad.  I think that code should only run when aParent is null...
Flags: needinfo?(ehsan)
Assignee: nobody → ehsan
Pushed by
Only inform windows that their parent is changing when being removed from their parent r=bzbarsky
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Please request Beta approval on this when you get a chance.
Flags: needinfo?(ehsan)
Keywords: regression
Comment on attachment 9020846 [details]
Bug 1500543 - Only inform windows that their parent is changing when being removed from their parent

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1498720

User impact if declined: This is actually probably bad, although I can't really say exactly what the user facing impact would be.  We end up creating windows in cases where we shouldn't.  The impact of that is kind of hard to reason about.  But it's a bit frightening.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This was a simple mistake with a simple localized fix, the fix isn't really risky and is well understood.

String changes made/needed: None
Flags: needinfo?(ehsan)
Attachment #9020846 - Flags: approval-mozilla-beta?
Comment on attachment 9020846 [details]
Bug 1500543 - Only inform windows that their parent is changing when being removed from their parent

[Triage Comment]
Fixes at a minimum a really spammy console message, maybe worse. Approved for 64.0b7.
Attachment #9020846 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.