701591 - tab label for data: URL on DOMContentLoaded is still "Connecting…"

Status: NEW

(Core :: DOM: Core & HTML, defect, P5)

Description

[Myk Melez [:myk] [@mykmelez]]

When loading a data URL like "data:text/html,<title>tab 1</title>" in a tab, the label of the tab is still "Connecting…" on DOMContentLoaded.  For non-data: URLs, however, the label is the title of the page by that point, and that is what I would expect to be the case for data: URLs as well.

A tab label gets set to the title of a page being loaded in the tab in two ways:

1. on DOMTitleChanged, which is dispatched by nsDocument::DoNotifyPossibleTitleChange <http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#5291> and handled by tabbrowser::DOMTitleChanged <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2653>;

2. in tabbrowser::mTabProgressListener::onStateChange <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#580>, if the title is still "Connecting…" when (aStateFlags & nsIWebProgressListener.STATE_STOP && aStateFlags & nsIWebProgressListener.STATE_IS_NETWORK).

Perhaps this is because DOMTitleChanged is dispatched asynchronously, while DOMContentLoaded is dispatched synchronously for data: URLs per nsXMLDocument::EndLoad <http://mxr.mozilla.org/mozilla-central/source/content/xml/document/src/nsXMLDocument.cpp#585> and thus DOMContentLoaded arrives before DOMTitleChanged (and before nsIWebProgressListeners are notified)?

Gavin noted in IRC that MXR blame suggests bug 461555 is related and sicking might know more, so cc:ing him.

sicking: is this the intended behavior (resolution: invalid), an unfortunate but unfixable side effect of other intended behavior (resolution: wontfix), or is there perhaps some way to solve this (dispatch DOMContentLoaded for data: URLs asynchronously, dispatch DOMTitleChanged for them synchronously, etc.)?

Comment 1

[Eddy Bruel [:ejpbruel]]

I talked to sicking on this. He suspects that it's a docshell problem, and it might be fixable. Will investigate further.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

I see this problem in the SDK tests (with the fix for bug 682681), and I'm trying to reduce a testcase, but I'm having trouble doing so.  Here is the current version of the testcase (paste this into the Error Console's code evaluation field and evaluate it):

var tabbrowser = Components.classes["@mozilla.org/appshell/window-mediator;1"].
getService(Components.interfaces.nsIWindowMediator).
getMostRecentWindow("navigator:browser").gBrowser;
var tab = tabbrowser.addTab();
tabbrowser.selectedTab = tab;
var browser = tabbrowser.getBrowserForTab(tab);
browser.addEventListener("DOMContentLoaded", function() { alert(tab.label) }, true, true);
tabbrowser.loadURI("data:text/html,<title>tab 1</title>");

This testcase correctly alerts "tab 1", though, and I'm not sure why.

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

ejpbruel pinged me about this on IRC. After looking into tabs API I noticed that we use 'DOMContentLoaded' event listener on browser:

https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/tabs/tab.js#L80
https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/tabs/events.js#L42

which is exactly what myks reduced case does, but in contrast to his test case we use tab's contentDocument.title for a tab label:

https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/tabs/tab.js#L136

I have modified test case from myk to match that:

var tabbrowser = Components.classes["@mozilla.org/appshell/window-mediator;1"].
getService(Components.interfaces.nsIWindowMediator).
getMostRecentWindow("navigator:browser").gBrowser;
var tab = tabbrowser.addTab();
tabbrowser.selectedTab = tab;
var browser = tabbrowser.getBrowserForTab(tab);
browser.addEventListener("DOMContentLoaded", function() { alert(browser.contentDocument.title) }, true, true);
tabbrowser.loadURI("data:text/html,<title>tab 1</title>");

unfortunately it seems to work fine here as well

Comment 4

[Myk Melez [:myk] [@mykmelez]]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #3)
> ejpbruel pinged me about this on IRC. After looking into tabs API I noticed
> that we use 'DOMContentLoaded' event listener on browser:
> 
> https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/tabs/
> tab.js#L80
> https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/tabs/
> events.js#L42
> 
> which is exactly what myks reduced case does, but in contrast to his test
> case we use tab's contentDocument.title for a tab label:
> 
> https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/tabs/
> tab.js#L136
> 
> I have modified test case from myk to match that:

My testcase uses tab.label rather than contentDocument.title intentionally, because this bug is about tab.label being wrong.

It's true that the current version of the SDK uses contentDocument.title, but that's because I haven't landed the fix for bug 682681 yet; and the reason I haven't landed that fix, which makes the SDK use tab.label, is because doing so triggers this bug. :-)

So that is why the testcase uses tab.label: in order to trigger this bug.  I still don't know why the testcase fails, though.

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Looking more into the code for the test that fails: https://github.com/mozilla/addon-sdk/blob/master/packages/addon-kit/tests/test-windows.js#L52

It looks like actual test case is this rather than one above.

var browser = Components.classes["@mozilla.org/appshell/window-mediator;1"].
              getService(Components.interfaces.nsIWindowMediator).
              getMostRecentWindow("navigator:browser");
var win = browser.openDialog("chrome://browser/content/browser.xul", "_blank",
                                "chrome,all,dialog=no",
                                "data:text/html,<title>windows API test</title>");

function onLoad() {
  var tabbrowser = win.gBrowser;
  var tab = tabbrowser.selectedTab;
  var browser = tabbrowser.getBrowserForTab(tab);
  browser.addEventListener('DOMContentLoaded', function() {
    alert('document title: ' + browser.contentDocument.title + '\n' +
          'tab label: ' + tab.label);
  }, true);
}

if (win.document.readyState !== 'complete')
  win.addEventListener('load', onLoad, false);
else
  onLoad();

Still it seems to work as expected, so I don't know what could it be.

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Yei!!! Finally I managed to create actual test case :)

var browser = Components.classes["@mozilla.org/appshell/window-mediator;1"].
              getService(Components.interfaces.nsIWindowMediator).
              getMostRecentWindow("navigator:browser");
var win = browser.openDialog("chrome://browser/content/browser.xul", "_blank",
                                "chrome,all,dialog=no",
                                "data:text/html,<title>tab 1</title>");

function onLoad() {
  var tabbrowser = win.gBrowser;
  var tab1 = tabbrowser.selectedTab;
  var browser1 = tabbrowser.getBrowserForTab(tab1);
  var tab2 = tabbrowser.addTab('data:text/html,<title>tab 2</title>');
  var browser2 = tabbrowser.getBrowserForTab(tab2);
  browser2.addEventListener('DOMContentLoaded', function() {
    alert('document title: ' + browser1.contentDocument.title + '\n' +
          'tab label: ' + tab1.label);
  }, true);
}

if (win.document.readyState !== 'complete')
  win.addEventListener('load', onLoad, false);
else
  onLoad();


It looks like if you open another tab in just opened browser new tab will load before the first tab. Also somehow timing is such that first tab's document has title set to it's document, but since no event was dispatched tab.label is not changed.

I guess test should not assume that first tab is loaded firs even though it was first :)

Comment 7

[Eddy Bruel [:ejpbruel]]

It looks like Myk's hypothesis is correct. I've added a couple of log statements, and DOMContentLoaded is received before any DOMTitleChanged events are received. Since DOMContentLoaded is fired synchronously, this is to be expected.

The next step is to figure out what the best way around this problem is. Myk already suggested the two obvious approaches. Intuitively, I expect that changing DOMTitleChanged to be fired synchronously is the harder approach, since it's not obvious that we're in a document that was loaded from a data URL at the point it is fired. Changing DOMContentLoaded to be fired asynchronously should be easy in theory, except there is probably a good reason why we are loading data url's synchronously in the first place. I'd like to figure that out first.

Comment 8

[Eddy Bruel [:ejpbruel]]

Smaug pointed out that DOMContentLoaded should in fact be fired asynchronously for data URL's in general. It should only be fired synchronously for data *documents*, which are a different concept (a data document is a document without a graphical representation, which is not the case here, since we're loading a document in a tab to be displayed).

I have tried to put some breakpoints in the code to figure out what's going on, but since Irakli's test is probably faulty (see comment 6), I've had to fall back to the test in test-windows.js, and I'm running into problems here with cfx test. It's not possible to attach gdb to cfx test directly. The --no-run option is provided as a work around for this, generating a command that supposedly yields the same results when entered in the shell, except it doesn't. When running the command generated with the --no-run flag turned on, I don't see any failing tests, or at least no output indicating that a test has failed, something that *is* visible when running cfx test directly.

I've mailed Myk, Irakli and Alex to ask for some clarification on this issue.

Comment 9

[Eddy Bruel [:ejpbruel]]

ochameau confirmed that when running cfx test directly, via --no-run, stdout doesn't work on Windows. He doesn't have an OSX machine available, but its possible that the same bug causes stdout not to work on OSX. We should look into this. In the meantime, I'll try to find a way around the problem.

Comment 10

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.