836567 - 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)

Status: RESOLVED FIXED

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

Description

[i.cyberface]

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

Comment 1

[(mostly gone) XtC4UaLL [:xtc4uall]]

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

Comment 2

[[:Aleksej]]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Hallvord R. M. Steen [:hallvors]]

Attachment: TC 836567.htm

Comment 5

[Hallvord R. M. Steen [:hallvors]]

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.).

Comment 6

[Boris Zbarsky [: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.

Comment 7

[Hallvord R. M. Steen [:hallvors]]

(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?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Tests for which?

Comment 9

[Hallvord R. M. Steen [:hallvors]]

(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?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 11

[Hallvord R. M. Steen [:hallvors]]

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?

Comment 12

[Hallvord R. M. Steen [:hallvors]]

..and I'll pester you for answers again, Boris. Sorry ;-]

Comment 13

[Boris Zbarsky [: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.

Comment 14

[Hallvord R. M. Steen [:hallvors]]

(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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 16

[Hallvord R. M. Steen [:hallvors]]

A better test for this is coming up in 
https://github.com/w3c/web-platform-tests/tree/hallvors/iframe-javascript-url
and should end up here when merged:
http://w3c-test.org/html/semantics/embedded-content/the-iframe-element/iframe_javascript_url_01.htm

Comment 19

[Boris Zbarsky [:bzbarsky]]

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...

Comment 20

[Samael Wang [:freesamael] (away for now)]

Attachment: wip.diff

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.

Comment 21

[Samael Wang [:freesamael] (away for now)]

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

Comment 22

[Samael Wang [:freesamael] (away for now)]

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...

Comment 23

[Samael Wang [:freesamael] (away for now)]

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

Comment 24

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 25

[Boris Zbarsky [: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.

Comment 26

[Samael Wang [:freesamael] (away for now)]

(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.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 28

[Samael Wang [:freesamael] (away for now)]

(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.

Comment 29

[Samael Wang [:freesamael] (away for now)]

Attachment: Bug 836567 - Part 1: Set ResultPrincipalURI to active document's URI before evaluating script.

Review commit: https://reviewboard.mozilla.org/r/199004/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199004/

Comment 30

[Samael Wang [:freesamael] (away for now)]

Attachment: Bug 836567 - Part 2: Don't switch away from preload browser on loading javascript: URIs.

When loading happens in a preloaded about:newtab tab, it would trigger
RedirectLoad to switch process. This is not appropriate for javascript: URIs,
since it's supposed to take side effects on current page.

Review commit: https://reviewboard.mozilla.org/r/199006/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199006/

Comment 31

[Samael Wang [:freesamael] (away for now)]

Attachment: 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.

Some notes about the changes:

Both test_bug384014.html and test_bug123696.html were testing reloading of
javascript: URI. The expected result of the iframes after reloading would
become about:blank. I deleted both file and instead wrote with a
web-platform-test to cover reloading of javascript: URI since wpt is more
preferable.

storage-cache-error.html was utilizing javascript: URI to test bug 1262766.
javascript: URI would cause CacheStorage::Keys throw a dom security exception
as it's null principal. With my patches the iframe's URL would no longer be
the javascript: URI, so it's no longer applicable for the test case. Instead we
can test what bug 1262766 was originally about - that CacheStorage::Keys would
throw a dom security exception if it's in a private browsing window.

Review commit: https://reviewboard.mozilla.org/r/199008/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199008/

Comment 32

[Boris Zbarsky [:bzbarsky]]

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.

Comment 33

[Boris Zbarsky [:bzbarsky]]

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/?

Comment 34

[Boris Zbarsky [:bzbarsky]]

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 35

[Boris Zbarsky [:bzbarsky]]

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/

Comment 36

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8927737 [details]
Bug 836567 - Part 1: Set ResultPrincipalURI to active document's URI before evaluating script.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199004/diff/1-2/

Comment 37

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8927738 [details]
Bug 836567 - Part 2: Don't switch away from preload browser on loading javascript: URIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199006/diff/1-2/

Comment 38

[Samael Wang [:freesamael] (away for now)]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199008/diff/1-2/

Comment 39

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8927737 [details]
Bug 836567 - Part 1: Set ResultPrincipalURI to active document's URI before evaluating script.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199004/diff/2-3/

Comment 40

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8927738 [details]
Bug 836567 - Part 2: Don't switch away from preload browser on loading javascript: URIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199006/diff/2-3/

Comment 41

[Samael Wang [:freesamael] (away for now)]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199008/diff/2-3/

Comment 42

[Samael Wang [:freesamael] (away for 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/#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 43

[Samael Wang [:freesamael] (away for now)]

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 44

[Samael Wang [:freesamael] (away for now)]

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...

Comment 45

[Boris Zbarsky [:bzbarsky]]

I'm sorry again for the horrible lag... Reviewing now.

Comment 46

[Boris Zbarsky [:bzbarsky]]

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

Comment 47

[Boris Zbarsky [:bzbarsky]]

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

Comment 48

[Boris Zbarsky [:bzbarsky]]

You should probably send an intent mail if you haven't yet...

Comment 49

[Samael Wang [:freesamael] (away for now)]

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...

Comment 50

[Samael Wang [:freesamael] (away for now)]

I really wanted to land this one...

Comment 51

[Boris Zbarsky [:bzbarsky]]

I'll pick this up and drive it over the line.  Thank you for all the work you've already put in!

Comment 52

[Jose María Acuña]

Any advance?

Comment 54

[Jose María Acuña]

Something new?

Comment 55

[Domenic Denicola]

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.

Comment 56

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 836567 - Part 1: Set ResultPrincipalURI to active document's URI before evaluating a javascript: URL.

Comment 57

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 836567 - Part 2: Replace inapplicable tests with a web-platform-test for reloading after setting javascript: URI, and fix other tests relying on javascript: URI.

Some notes about the changes:

Both test_bug384014.html and test_bug123696.html were testing reloading of
javascript: URI. The expected result of the iframes after reloading would
become about:blank. I deleted both file and instead wrote with
web-platform-test to cover reloading of javascript: URI since wpt is more
preferable.

storage-cache-error.html was utilizing javascript: URI to test bug 1262766.
javascript: URI would cause CacheStorage::Keys throw a dom security exception
as it's null principal. With my patches the iframe's URL would no longer be
the javascript: URI, so it's no longer applicable for the test case. Instead we
can test what bug 1262766 was originally about - that CacheStorage::Keys would
throw a dom security exception if it's in a private browsing window.

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7ec30cb5c9c
Part 1: Set ResultPrincipalURI to active document's URI before evaluating a javascript: URL. r=smaug
https://hg.mozilla.org/integration/autoland/rev/68447a9ccb6a
Part 2: Replace inapplicable tests with a web-platform-test for reloading after setting javascript: URI, and fix other tests relying on javascript: URI. r=Gijs,miker

Comment 59

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/21165 for changes under testing/web-platform/tests

Comment 60

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 61

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/a7ec30cb5c9c
https://hg.mozilla.org/mozilla-central/rev/68447a9ccb6a

Comment 62

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 63

[Eric Shepherd [:sheppy]]

// deleted

Comment 64

[Boris Zbarsky [:bzbarsky]]

Eric, was comment 63 supposed to go on a different bug?

Comment 65

[Eric Shepherd [:sheppy]]

Yep. Not sure how that happened. Thanks for catching it.

Comment 66

[Eric Shepherd [:sheppy]]

Would the following be a reasonable summary of the changes as applied to documentation?

• This is a bug fix that only needs to be mentioned in release notes/Fx74 for developers, rather than requiring other documentation changes.
• Would it be correct to state this in that page as "When loading and executing a script using the javascript: URL scheme, the script's principal is now correctly initialized to be the active document's URI, regardless of document nesting depth" or something similar?

Comment 67

[Boris Zbarsky [:bzbarsky]]

This is a bug fix that only needs to be mentioned in release notes/Fx74 for developers, rather than requiring other documentation changes.

That depends on whether anything documented the URL of "javascript:" result documents....

Would it be correct to state this in that page as

That's not an accurate description of the change. The change concerns the case when you evaluate a "javascript:" URL and the resulting value is a string. In that case, that string is parsed to create an HTML document. The URL of that document used to be the "javascript:" URL that returned the string, but in this bug that was changed to the URL of the document that the "javascript:" URL was evaluated in. The only change is what the URL is; there are no changes to principals (origins) involved.

Comment 68

[Eric Shepherd [:sheppy]]

Wow - I'm glad I asked. I never in a million years would have gotten that from anything said in these comments or by reading the code. I mean, maybe if I'd spent a month or two reading the code I'd have figured it out, but that's not the kind of time I could have given it. Glad I asked. :)

I have now added a note to Firefox 74 for developers that reads as follows:

When a JavaScript URL (javascript:) is evaluated and the result is a string, this string is parsed to create an HTML document, which is then presented. Previously, this document's URL (as reported by the document.location property, for example) was the originating javascript: URL; it is now correctly the URL of the originating document (bug 836567).

After substantial searching, I've found nowhere else that gets into these URLs in any real depth, so this should be all that's needed for this bug. Marking it as documentation complete; please let me know if this is unsatisfactory, or feel free to update the text yourself.

Comment 69

[Boris Zbarsky [:bzbarsky]]

Perfect, thank you! I made a minor tweak to the "originating document" bit, but otherwise looks good. And thank you for checking for other docs on this!