Closed Bug 1179058 Opened 4 years ago Closed 4 years ago

~18,000 instances of 'WARNING: NS_ENSURE_SUCCESS(rv, true) failed with result 0x80570030' emitted from docshell/base/nsDocShell.cpp during linux64 debug testing

Categories

(Core :: DOM: Navigation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: erahm, Assigned: nhnt11)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

> 17724 [NNNNN] WARNING: NS_ENSURE_SUCCESS(rv, true) failed with result 0x80570030: file docshell/base/nsDocShell.cpp, line 11758

This is the #1 most verbose warning [1] during linux64 debug testing. It seems to have appeared in the last week. 0x80570030 is NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED.

It shows up primarily during e10s tests:

> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-4-bm122-tests1-linux64-build6.txt:3997
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-3-bm115-tests1-linux64-build6.txt:3643
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-5-bm122-tests1-linux64-build10.txt:3497
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-1-bm115-tests1-linux64-build2.txt:2757
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-2-bm123-tests1-linux64-build1.txt:2674
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-3-bm53-tests1-linux64-build6.txt:562
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-1-bm52-tests1-linux64-build5.txt:384
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-2-bm122-tests1-linux64-build13.txt:209
> mozilla-central_ubuntu64_vm-debug_test-mochitest-other-bm115-tests1-linux64-build5.txt:1

This warning was introduced in bug 1118285 on 2015-06-29. My best guess is that the JS object implementing nsIXULBrowserWindow referenced in |nsContentTreeOwner::ShouldAddToSessionHistory| [2] has not defined a |shouldAddToSessionHistory| function.

[1] https://hg.mozilla.org/mozilla-central/annotate/291614a686f1/docshell/base/nsDocShell.cpp#l11758
[2] https://hg.mozilla.org/mozilla-central/annotate/291614a686f1/xpfe/appshell/nsContentTreeOwner.cpp#l472
Attached patch Patch (obsolete) — Splinter Review
This patch fixes the warnings on my local debug build.
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c77f1156434
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Attachment #8628487 - Flags: review?(adw)
Hmm, this won't work as expected since the content process's "copy" of NewTabURL at runtime will be different from chrome's.  An overridden URL in one won't be seen in the other.  (I verified by dump()ing NewTabURL.get() in tab-content.js after overriding the URL using the (chrome) browser console.  You can use the content toolbox too.)

We broke this in bug 1118285 but didn't realize it.  Before that, docshell checked browser.newtab.url itself, right?  Prefs storage is shared across the two processes, so it worked.

I think we're going to have to have NewTabURL send a message to content whenever the URL is overridden just so that this WebBrowserChrome.shouldAddToSessionHistory() can use it.  If necessary, in the meantime we can land an empty implementation of shouldAddToSessionHistory just to make the warnings go away.

needinfo'ing Florian for his thoughts.
Flags: needinfo?(florian)
Attachment #8628487 - Flags: review?(adw)
(In reply to Drew Willcoxon :adw from comment #2)
> Hmm, this won't work as expected since the content process's "copy" of
> NewTabURL at runtime will be different from chrome's.  An overridden URL in
> one won't be seen in the other.  (I verified by dump()ing NewTabURL.get() in
> tab-content.js after overriding the URL using the (chrome) browser console. 
> You can use the content toolbox too.)
> 
> We broke this in bug 1118285 but didn't realize it.  Before that, docshell
> checked browser.newtab.url itself, right?  Prefs storage is shared across
> the two processes, so it worked.
> 
> I think we're going to have to have NewTabURL send a message to content
> whenever the URL is overridden just so that this
> WebBrowserChrome.shouldAddToSessionHistory() can use it.  If necessary, in
> the meantime we can land an empty implementation of
> shouldAddToSessionHistory just to make the warnings go away.
> 
> needinfo'ing Florian for his thoughts.

So this is the rare instance where a warning is actually useful. I don't think we should silence it, perhaps we should backout bug 1118285 instead if a fix is going to take some time.
(In reply to Drew Willcoxon :adw from comment #2)

> I think we're going to have to have NewTabURL send a message to content
> whenever the URL is overridden just so that this
> WebBrowserChrome.shouldAddToSessionHistory() can use it.  If necessary, in
> the meantime we can land an empty implementation of
> shouldAddToSessionHistory just to make the warnings go away.

Assuming the session history is handled by the content process and the warning isn't just noise, I'm afraid we'll have to to this, yes.
Flags: needinfo?(florian)
Attached patch Patch 2Splinter Review
This reverts the interface changes in bug 1118285 and makes nsDocShell check specifically for about:newtab. This is a short term fix. For the long term we likely want NewTabURL to pass messages to content when the URL changes.
Attachment #8628487 - Attachment is obsolete: true
Attachment #8628974 - Flags: review?(adw)
Attachment #8628974 - Flags: review?(adw) → review+
(In reply to Drew Willcoxon :adw from comment #2)
> Hmm, this won't work as expected since the content process's "copy" of
> NewTabURL at runtime will be different from chrome's.

Good catch. Probably worth filing a bug to somehow make NewTabURL.jsm throw an exception or warning when used from a content process.
https://hg.mozilla.org/mozilla-central/rev/ea4f4e3b0090
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.