Open Bug 836567 Opened 7 years ago Updated 11 months ago

Set document.URL (location) to url of active document when navigation started for javascript:-generated documents per HTML5 (Nested javascript: iframe doesn't load, runs into nesting limit for frames with same URL)

Categories

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

x86
All
defect

Tracking

()

People

(Reporter: i.cyberface, Assigned: bzbarsky)

References

(Blocks 3 open bugs, )

Details

(Keywords: dev-doc-needed, Whiteboard: DUPEME?)

Attachments

(4 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_2) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.56 Safari/537.17

Steps to reproduce:

I'm creating an iframe with javascript and set iframe's content using `javascript:` scheme: first I create iframe's html I then assign this content to the iframe's window using iframe.contentWindow property. This techniques ensures that all javascript inside the iframe is initialised synchronously in all browsers. The iframe loads correctly but it also creates nested iframe using javascript, and this nested iframe is not initialised

Here is fiddle http://jsfiddle.net/chopachom/zKxZP/7/

Tested in IE10, Chrome, Opera 12.12 - works as expected




Actual results:

The inner iframe which inserted inside another iframe is not loaded


Expected results:

Inner iframe should have loaded
Also reproducible with Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.28) Gecko/20120306 Firefox/3.6.28 ID:20120306064154
Reproducible also with 2005-08-01-05-trunk-firefox-1.0+.en-US.linux-i686.
Builds from 2004 show no content on the page at all.
OS: Mac OS X → All
Whiteboard: DUPEME?
Version: 18 Branch → 1.5.0.x Branch
Component: Untriaged → DOM
Product: Firefox → Core
Version: 1.5.0.x Branch → 1.7 Branch
Presumably this is getting blocked by the "Loading an iframe with the same URI as an ancestor" recursion protection code.  Should check what the spec says on that end nowadays.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Since IE now handles that TC fine, maybe it's now time to up the limit from 1 to 2 or 3? It's a one-letter fix here:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#125

Safe?

(I didn't spend enough time on this to figure out what older tests are relevant, so I don't know what IE's current limit is - all the tests I found were pretty obscure and time consuming to interpret. We had good tests at Opera but I don't know if they are public - can't find them in the presto-testo repo.).
Summary: Nested iframe doesn't load when changing the iframe's src → Nested iframe doesn't load when changing the iframe's src, runs into nesting limit for frames with same URL
Flags: needinfo?(bzbarsky)
> Since IE now handles that TC fine

How does it handle <iframe src="same-url-as-main-page">?

I any case, what would "fix" this particular testcase is not using the javascript: URI as the location of the page javascript: creates.  Pretty sure we have an existing bug on that.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #6)
> How does it handle <iframe src="same-url-as-main-page">?

Blocked. So you're right, the javascript: stuff is special-cased.

> I any case, what would "fix" this particular testcase is not using the
> javascript: URI as the location of the page javascript: creates.  Pretty
> sure we have an existing bug on that.

So this is a dup - do we have good tests?
Tests for which?
(In reply to Boris Zbarsky [:bz] from comment #8)
> Tests for which?

Sorry - I meant tests for the bug this one is supposedly a duplicate of ;)

Do you by any chance manage to find that bug, BTW?
> Sorry - I meant tests for the bug this one is supposedly a duplicate of ;)

Doubt it, since we wouldn't pass them right now!

> Do you by any chance manage to find that bug, BTW?

I hadn't looked.  But I don't see an obvious bug filed for it, which just tells me I'm using the wrong search terms.
I did a bit of digging. Basically, in dom/jsurl/nsJSProtocolHandler.cpp we reach

nsJSURI::EqualsInternal

comparing the URLs of the outer nested IFRAME - javascript:window["html"] - and the inner nested IFRAME - javascript:window["html"].

This code would conclude differently if protocol + "path" were not exactly the same:

    // Compare the member data that our base class knows about.
    if (!nsSimpleURI::EqualsInternal(otherJSURI, aRefHandlingMode)) {
        *aResult = false;
        return NS_OK;
    }

This strikes me as somewhat strange. The test passes if you just change a single letter in the variable name of either the outer or the inner nested frame - but that's hacking around a nonsensical limitation in the engine. These URLs run in different windows, and being letter-by-letter the same is pretty much irrelevant. I suggest this code either 

a) also compares the global object and only considers javascript: URLs "same" if they are exact same string and have the same associated global object

or 

b) decide that comparing JS URLs for equality is somewhat odd anyway and just always return false.

I'd do b) for simplicity, but I don't know other places this code may be called from.. are there any?
..and I'll pester you for answers again, Boris. Sorry ;-]
Flags: needinfo?(bzbarsky)
> but I don't know other places this code may be called from.. are there any?

Tons.  Lots of things rely on URI equality actually working.  You do not want to mess with that.

Again, the right fix here is updating to the HTML5 spec's handling of what the document URI ends up being after navigation to a javascript: URI.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #13)
> > but I don't know other places this code may be called from.. are there any?
> 
> Tons.  Lots of things rely on URI equality actually working.  You do not
> want to mess with that.

As a general case, URI equality is important - sure. But how does it make any sense to compare javascript: URIs from different windows for *string* equality? The variables they reference are from different global objects, the generated HTML code may or may not be entirely different. Having actually stepped through this code in a debugger it looks more like we copied over the logic from comparing normal URLs without really considering the special nature of javascript: ones.

> Again, the right fix here is updating to the HTML5 spec's handling of what
> the document URI ends up being after navigation to a javascript: URI.

Per HTML5 the address of a javascript: URI-generated document should be set to about:blank. So at the point where we actually load the inner IFRAME's javascript: URL, we'll be comparing it against the outer IFRAME's about:blank URL and it will work. (This explanation is thanks to Anne patiently clarifying things on IRC).

And I can't find any other bug about this either, so I'll morph this to The Bug for now.
Summary: Nested iframe doesn't load when changing the iframe's src, runs into nesting limit for frames with same URL → Set document.URL to about:blank for javascript:-generated documents per HTML5 (Nested javascript: iframe doesn't load, runs into nesting limit for frames with same URL)
> But how does it make any sense to compare javascript: URIs from different windows for
> *string* equality?

How do you know they're from different windows?  The Equals() function is used from lots of places.  Thing like hashtable keys, for example.  Breaking it would really be a very very bad idea, trust me.

> Per HTML5 the address of a javascript: URI-generated document should be set to
> about:blank.

Not quite.  What the spec says in https://html.spec.whatwg.org/multipage/browsers.html#navigate step 15 substep 12 is:

  When it comes time to set the document's address in the navigation algorithm, use
  address as the override URL.

where in step 10 we had:

  Let address be the address of the active document of the browsing context being
  navigated.

In other words, it's supposed to keep whatever URL was there, which may or may not happen to be about:blank.
Summary: Set document.URL to about:blank for javascript:-generated documents per HTML5 (Nested javascript: iframe doesn't load, runs into nesting limit for frames with same URL) → Set document.URL to url of active document when navigation started for javascript:-generated documents per HTML5 (Nested javascript: iframe doesn't load, runs into nesting limit for frames with same URL)
Summary: Set document.URL to url of active document when navigation started for javascript:-generated documents per HTML5 (Nested javascript: iframe doesn't load, runs into nesting limit for frames with same URL) → Set document.URL (location) to url of active document when navigation started for javascript:-generated documents per HTML5 (Nested javascript: iframe doesn't load, runs into nesting limit for frames with same URL)
Assignee: nobody → sawang
Component: DOM → Document Navigation
Version: 1.7 Branch → unspecified
Component: Document Navigation → DOM: Core & HTML
Duplicate of this bug: 1281375
Duplicate of this bug: 1329602
Blocks: 1247968
For what it's worth, the right way to fix this is probably to wait for bug 1319111 to land and then set the relevant URL on the loadinfo for javascript: channels...
Attached patch wip.diff (obsolete) — Splinter Review
One thing I'm still thinking is what to do when you type a javascript: URL from about:home or other about: pages which shows empty URL bar. We were showing the javascript: URL before but with this patch it would still show empty.
Hi Boris,

I'm restarting to work on this bug, but I get stuck. 

When a user types an invalid javascript URL, or just javascript:void(0) on the address bar and then press enter, I want to change the displayed URL in address bar back to previous document's URL, so basically I'm trying to find out how to handle the error nsJSThunk::EvaluateScript returns. 

I was thinking of firing a dummy location change to update the address bar. The problem is that docshell is not aware of the NS_ERROR_DOM_RETVAL_UNDEFINED at all. It seems nsJSChannel is intentionally set as LOAD_BACKGROUND, so docshell wouldn't get OnStateChange or OnStopRequest when nsJSChannel is removed from the load group [1], and nsURILoader::m_targetStreamListener isn't set in this case so it wouldn't get notified from nsJSChannel::NotifyListener() either.

The LOAD_BACKGROUND hack in nsJSChannel is so confusing to me that I couldn't figure out what to do. Could you point me some directions?

[1] http://searchfox.org/mozilla-central/rev/423b2522c48e1d654e30ffc337164d677f934ec3/dom/jsurl/nsJSProtocolHandler.cpp#709
Flags: needinfo?(bzbarsky)
The last thing I tried was to turn nsJSChannel a normal LOAD_DOCUMENT_URI load, but then I don't know what to do with mStreamChannel which is also a document load...
I don't quite understand why we need to re-add the JSChannel [1]. Wouldn't the mStreamChannel->AsyncOpen [2] above add the stream channel to the load group and make the stream channel be able to receive cancellation of the load group?

[1] http://searchfox.org/mozilla-central/rev/af86a58b157fbed26b0e86fcd81f1b421e80e60a/dom/jsurl/nsJSProtocolHandler.cpp#783
[2] http://searchfox.org/mozilla-central/rev/af86a58b157fbed26b0e86fcd81f1b421e80e60a/dom/jsurl/nsJSProtocolHandler.cpp#773
> I want to change the displayed URL in address bar back to previous document's URL

Can we just handle this via the UI, not in the javascript: implementation guts?

> It seems nsJSChannel is intentionally set as LOAD_BACKGROUND

Right, so it doesn't thrash the throbber...

> so it wouldn't get notified from nsJSChannel::NotifyListener()

Well, more precisely because it has an error mStatus, right?

> I don't quite understand why we need to re-add the JSChannel

You're right that the stream channel will already be in the loadgroup.  But the consumer knows nothing about the stream channel.  It knows about us an the loadgroup.  If it cancels the loadgroup, our status better get updated to match the cancellation reason.  For that to happen, we have to be in the loadgroup too.
Flags: needinfo?(bzbarsky)
> When a user types an invalid javascript URL, or just javascript:void(0) on the address bar
> and then press enter, I want to change the displayed URL in address bar back to previous document's URL

Actually, why do we even want this behavior?  Seems fairly user-hostile to me.  To the extent that we allow typing javascript: into the URL bar at all, seems like you may want to type something in, see what it alerts or logs, then edit it, etc.  Losing the thing that was typed in is pretty suboptimal.  I know that's the behavior Chrome has, say, and I've always hated it there.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #25)
> > When a user types an invalid javascript URL, or just javascript:void(0) on the address bar
> > and then press enter, I want to change the displayed URL in address bar back to previous document's URL
> 
> Actually, why do we even want this behavior?  Seems fairly user-hostile to
> me.  To the extent that we allow typing javascript: into the URL bar at all,
> seems like you may want to type something in, see what it alerts or logs,
> then edit it, etc.  Losing the thing that was typed in is pretty suboptimal.
> I know that's the behavior Chrome has, say, and I've always hated it there.

I feel it's a bit confusing that nothing would happen after pressing enter, but I agree this can be handled via the UI. I'll keep the current behavior for this bug.
> I feel it's a bit confusing that nothing would happen after pressing enter

In what sense?  The whole point of evaluating javascript: in the URL bar is that it has side-effects; those presumably happen...
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #27)
> > I feel it's a bit confusing that nothing would happen after pressing enter
> 
> In what sense?  The whole point of evaluating javascript: in the URL bar is
> that it has side-effects; those presumably happen...

Ah. I thought the evaluation fails silently when the javascript: URL is incorrect, but we do provide error messages in web console.
Attachment #8871192 - Attachment is obsolete: true
Comment on attachment 8927737 [details]
Bug 836567 - Part 1: Set ResultPrincipalURI to active document's URI before evaluating script.

https://reviewboard.mozilla.org/r/199004/#review206186

I'm very sorry for the lag here.  I needed to work through the spec around this stuff carefully...

::: docshell/base/nsDocShell.cpp:11191
(Diff revision 1)
>  
>    if (aPrincipalToInherit) {
>      loadInfo->SetPrincipalToInherit(aPrincipalToInherit);
>    }
>  
> +  // If it's a javascript load, the document URI should be set to the previous

This is not what the spec says to do, right?

What the spec says to do is that the "navigate" action (which is where we are right now) queues a task to run some steps.  Those steps include "Let address be the URL of the active document of browsingContext."  But that's at the point when those (async!) steps execute, not now.

In other words, per spec the URL needed can be derived entirely from the document, and that should happen at a later point than this, during the async steps in the javascript: protocol.

Here's a testcase that this patch fails but Safari and Chrome handle per spec:

    <iframe></iframe>
    <script>
      frames[0].location.href = `javascript:"<span onclick='alert(document.URL)'>Click me and see what gets alerted</span>"`;
      frames[0].location.href += "#foo";
    </script>

The alert should say "about:blank#foo".

You can come up with similar testcases when using pushState/replaceState right after the start of navigation to javascript:.

Please add some tests along these lines.
Attachment #8927737 - Flags: review?(bzbarsky) → review-
Comment on attachment 8927738 [details]
Bug 836567 - Part 2: Don't switch away from preload browser on loading javascript: URIs.

https://reviewboard.mozilla.org/r/199006/#review206188

In an ideal world, maybe these would be checks for the URI_OPENING_EXECUTES_SCRIPT protocol flag, but I'm not actually sure.  The important part here is not the script-execution, but the "not actually changing what's going on" aspect of javasscript:...  So I'm not sure which is better.

I assume this was caught by tests?  If not, can we add a test?

r=me

::: browser/base/content/browser.js:1052
(Diff revision 1)
>  
>    let mustChangeProcess = requiredRemoteType != currentRemoteType;
>    let newFrameloader = false;
>    if (browser.getAttribute("isPreloadBrowser") == "true" && uri != "about:newtab") {
>      // Leaving about:newtab from a used to be preloaded browser should run the process
> -    // selecting algorithm again.
> +    // selecting algorithm again, unless it's a javascript: URI which should take

s/take/have/?
Attachment #8927738 - Flags: review?(bzbarsky) → review+
Comment on attachment 8927737 [details]
Bug 836567 - Part 1: Set ResultPrincipalURI to active document's URI before evaluating script.

https://reviewboard.mozilla.org/r/199004/#review206186

> This is not what the spec says to do, right?
> 
> What the spec says to do is that the "navigate" action (which is where we are right now) queues a task to run some steps.  Those steps include "Let address be the URL of the active document of browsingContext."  But that's at the point when those (async!) steps execute, not now.
> 
> In other words, per spec the URL needed can be derived entirely from the document, and that should happen at a later point than this, during the async steps in the javascript: protocol.
> 
> Here's a testcase that this patch fails but Safari and Chrome handle per spec:
> 
>     <iframe></iframe>
>     <script>
>       frames[0].location.href = `javascript:"<span onclick='alert(document.URL)'>Click me and see what gets alerted</span>"`;
>       frames[0].location.href += "#foo";
>     </script>
> 
> The alert should say "about:blank#foo".
> 
> You can come up with similar testcases when using pushState/replaceState right after the start of navigation to javascript:.
> 
> Please add some tests along these lines.

I guess to make this even clearer and avoid Gecko's other about:blank weirdness, you could do:

    <iframe></iframe>
    <script>
      onload = function() {
        //  frames[0].location.href = `javascript:"<span onclick='alert(document.URL)'>Click me and see what gets alerted</span>"`;
        frames[0].location.href += "#foo";
      }
    </script>

and at that point you can give the iframe any same-origin url.
Comment on attachment 8927739 [details]
Bug 836567 - Part 3: Replace inapplicable tests with a web-platform-test for reloading after setting javascript: URI, and fix other tests relying on javascript: URI.

https://reviewboard.mozilla.org/r/199008/#review206192

::: browser/base/content/test/urlbar/browser_urlbarCopying.js:285
(Diff revision 1)
>    }, cb, cb);
>  }
>  
>  function loadURL(aURL, aCB) {
>    BrowserTestUtils.loadURI(gBrowser.selectedBrowser, aURL);
> -  BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, false, aURL).then(aCB);
> +  BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, false).then(aCB);

Why is this change OK for the non-javascript: cases?  Should we just change what we pass as the third arg instead of removing it completely?

::: browser/base/content/test/urlbar/browser_urlbar_blanking.js:16
(Diff revision 1)
>      await BrowserTestUtils.removeTab(tab);
>    }
>  });
>  
>  add_task(async function() {
> +  // The test was originally about that reloading of a javascript: URL could

s/about that/to check that/

::: browser/base/content/test/urlbar/browser_urlbar_blanking.js:17
(Diff revision 1)
>    }
>  });
>  
>  add_task(async function() {
> +  // The test was originally about that reloading of a javascript: URL could
> +  // throw an error and empty the URL bar. The case no longer exists as in

s/The case no longer exists/This situation can no longer happen/

::: browser/base/content/test/urlbar/browser_urlbar_blanking.js:18
(Diff revision 1)
>  });
>  
>  add_task(async function() {
> +  // The test was originally about that reloading of a javascript: URL could
> +  // throw an error and empty the URL bar. The case no longer exists as in
> +  // bug 836567 we set document.URL to active document's URL on navigation of a

s/of a/to a/

::: browser/base/content/test/urlbar/browser_urlbar_blanking.js:19
(Diff revision 1)
>  
>  add_task(async function() {
> +  // The test was originally about that reloading of a javascript: URL could
> +  // throw an error and empty the URL bar. The case no longer exists as in
> +  // bug 836567 we set document.URL to active document's URL on navigation of a
> +  // javascript: URL, reloading after that will simply reload the originally

s/,/;/ and s/originally/original/

::: browser/base/content/test/urlbar/browser_urlbar_blanking.js:20
(Diff revision 1)
>  add_task(async function() {
> +  // The test was originally about that reloading of a javascript: URL could
> +  // throw an error and empty the URL bar. The case no longer exists as in
> +  // bug 836567 we set document.URL to active document's URL on navigation of a
> +  // javascript: URL, reloading after that will simply reload the originally
> +  // active document than the javascript: URL itself. But we can still verify

s/than/rather than/
Attachment #8927739 - Flags: review?(bzbarsky) → review+
Priority: -- → P2
Comment on attachment 8927737 [details]
Bug 836567 - Part 1: Set ResultPrincipalURI to active document's URI before evaluating script.

https://reviewboard.mozilla.org/r/199004/#review206186

> I guess to make this even clearer and avoid Gecko's other about:blank weirdness, you could do:
> 
>     <iframe></iframe>
>     <script>
>       onload = function() {
>         //  frames[0].location.href = `javascript:"<span onclick='alert(document.URL)'>Click me and see what gets alerted</span>"`;
>         frames[0].location.href += "#foo";
>       }
>     </script>
> 
> and at that point you can give the iframe any same-origin url.

I moved the logic into nsJSChannel, right before mIOThunk->EvaluateScript. This implies if `nsJSChannel::mIsAsync` is `false` then it will still be evaluated synchronously. I only found one consumer setting it to false:
https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/dom/plugins/base/nsPluginHost.cpp#3192

The web-platform-test `location_reload_javascript_url.html` in part 3 is modified to cover the test case commented.
Comment on attachment 8927738 [details]
Bug 836567 - Part 2: Don't switch away from preload browser on loading javascript: URIs.

https://reviewboard.mozilla.org/r/199006/#review206188

Added `browser_aboutnewtab_javascript_uri.js` for this case.
Comment on attachment 8927739 [details]
Bug 836567 - Part 3: Replace inapplicable tests with a web-platform-test for reloading after setting javascript: URI, and fix other tests relying on javascript: URI.

https://reviewboard.mozilla.org/r/199008/#review206192

I need to take an English course...
I'm sorry again for the horrible lag... Reviewing now.
Comment on attachment 8927737 [details]
Bug 836567 - Part 1: Set ResultPrincipalURI to active document's URI before evaluating script.

https://reviewboard.mozilla.org/r/199004/#review212590

r=me
Attachment #8927737 - Flags: review?(bzbarsky) → review+
Comment on attachment 8927739 [details]
Bug 836567 - Part 3: Replace inapplicable tests with a web-platform-test for reloading after setting javascript: URI, and fix other tests relying on javascript: URI.

https://reviewboard.mozilla.org/r/199008/#review212592
You should probably send an intent mail if you haven't yet...
It apparently fails dom/base/test/test_bug353334.html
https://treeherder.mozilla.org/logviewer.html#?job_id=151344424&repo=try&lineNumber=4745

Trying to figure it out...
I really wanted to land this one...
Assignee: freesamael → nobody
I'll pick this up and drive it over the line.  Thank you for all the work you've already put in!
Assignee: nobody → bzbarsky
Any advance?
Duplicate of this bug: 1464561
Something new?
See Also: → 1504035
The spec here has slightly simplified in https://github.com/whatwg/html/pull/4205, with tests introduced at https://github.com/web-platform-tests/wpt/pull/14316.
You need to log in before you can comment on or make changes to this bug.