Closed Bug 1297125 Opened 8 years ago Closed 8 years ago

Different browser behaviour for document.write in error handlers

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Version:
47 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: bugzilla, Assigned: bzbarsky)

References

Details

Attachments

(2 files, 1 obsolete file)

testcase from comment #0
241 bytes, text/html
Details
Rebased on top of bug 619109
17.60 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160604131506

Steps to reproduce:

This HTML produces different results in Chrome & Safari on the one hand and Firefox on the other:

<html>
<head>
  <script>
    errorHandler = function() {
      document.write("<h1>Hello</h1>");
    }
  </script>
  <script src='//invalid-domain.xyz/nope' onerror='errorHandler()'></script>
</head>
<body>
  <h1>World!</h1>
</body>
</html>


Actual results:

By the time the error handler runs, Firefox has already closed the document and it thus overwrites all existing content by re-opening it. The final result is "Hello"


Expected results:

In Chrome & Safari the document is still open, and thus the final result is "Hello World!". Which is the correct behaviour? Is Firefox's behaviour caused by the speculative parsing? And in the mean time, what can I do? Is there a way to find out whether the document has already been closed? Are there any other method that allow synchronous execution in this error handler and, in particular, the synchronous addition of a new script node to be executed before the next one?
confirmed on Nightly 2016-08-21
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM
Ever confirmed: true
Product: Firefox → Core
This is an extremely simplified example - using createTextNode and placing it into the DOM would be the right thing to do here. But the idea is rather to have an error handler that replaces/reloads the script tag with a new url (changing the src value does not reload it). The challenge is that the contents of both scripts use document.write which is fine if the original call suceeds, but breaks down if the error handler is called. 

    errorHandler = function() {
      document.write("<script src='//valid-domain.xyz/yep.js'></script>");
    }

If we use document.write (which works fine in Chrome & Safari), we erase the document in Firefox and if use appendChild/insertBefore the document.write inside the called script will wipe the site on all browsers. Any help?

Also, why does this simple looking example lead to different behaviours in different browsers? Can the document really be closed before understanding the DOM-side-effects of all event listeners such as error handlers?
Any ideas, Samael?
Flags: needinfo?(sawang)
Was it unexpected? In HTML4 it mentions this note in HTML scripting [1]
> Note that "document.write" or equivalent statements in intrinsic event handlers 
> create and write to a new document rather than modifying the current one.

HTML5 uses more generic description that document.open() should be invoked 
"If the insertion point is undefined" [2]. Since the parser could reach EOF 
before event dispatching, insertion point being undefined looks reasonable to me.

Any other thoughts here?

BTW if you do want to modify the document in an event handler, wouldn't using 
'document.body.innerHTML += ...' be more reliable?

[1] https://www.w3.org/TR/html4/interact/scripts.html#h-18.2.3
[2] https://www.w3.org/TR/html/webappapis.html#documentwrite
Flags: needinfo?(sawang)
.innerHTML will not load <script> nodes, which is explicitly goal.
(In reply to Samael Wang [:freesamael][:sawang] from comment #5)
> Was it unexpected? In HTML4 it mentions this note in HTML scripting [1]
> > Note that "document.write" or equivalent statements in intrinsic event handlers 
> > create and write to a new document rather than modifying the current one.
> 
> HTML5 uses more generic description that document.open() should be invoked 
> "If the insertion point is undefined" [2]. Since the parser could reach EOF 
> before event dispatching, insertion point being undefined looks reasonable
> to me.
> 
> Any other thoughts here?
> 

What do you think, Boris?

> BTW if you do want to modify the document in an event handler, wouldn't
> using 
> 'document.body.innerHTML += ...' be more reliable?
> 
> [1] https://www.w3.org/TR/html4/interact/scripts.html#h-18.2.3
> [2] https://www.w3.org/TR/html/webappapis.html#documentwrite
Flags: needinfo?(bzbarsky)
In terms of the spec, we go through the script loading stuff and end up at https://html.spec.whatwg.org/multipage/syntax.html#scriptEndTag and end up with a "pending parsing-blocking script".  Then step 8 of the "Otherwise" steps there should land us at https://html.spec.whatwg.org/multipage/scripting.html#execute-the-script-block which should fire the error event in step 2.

OK, what about the insertion point bit?  This is maintained by the parser, back in 
https://html.spec.whatwg.org/multipage/syntax.html#scriptEndTag and is defined around the entire call to <https://html.spec.whatwg.org/multipage/scripting.html#execute-the-script-block>, so per spec document.write in this error listener should not blow away the document as far as I can tell.

In terms of our implementation, for this specific case we fire the error event from nsScriptElement::ScriptAvailable.  That's outside the BeginEvaluating/EndEvaluating stuff on nsIScriptElement which flags the parser as having a defined insertion point.  In particular, for this error case it gets called from nsScriptLoader::OnStreamComplete directly.

Fixing this for the onerror case is pretty simple: we can just twiddle the parser state to give it a defined insertion point around the call to FireScriptAvailable in the mParserBlockingRequest == request case in OnStreamComplete.

But per spec, I believe doing a document.write from the _load_ event of the script should also avoid nuking the document.  Worth checking what other UAs do there.  So it may make more sense to pass a boolean arg to all the OnScriptAvailable stuff that indicates whether the script is mParserBlockingRequest or not.

Another alternative may be to just twiddle mCreatorParser if there is one (I _think_ that should correspond to the cases we care about) around firing the script's load/error events, but only in the case when we're coming from this sync path.
Flags: needinfo?(bzbarsky)
Oh, and I guess the load event is not fired from OnScriptAvailable anyway.  So really, we should do the twiddling in OnStreamComplete and then fix the onload case separately (probably by changing ProcessRequest to twiddle again around whatever notification it is that fires the load event in the end (FireScriptEvaluated?); we would again need to check for the script being the parser-blocking one, but we could pass that boolean in from the caller in nsScriptLoader::ProcessPendingRequests.

Even better would be general changes to align better with the spec here, but that's especially hard given the before/afterscriptexecute bits, during which I think we _do_ want the insertion point to be undefined.
From a practical point of view, are there any modifications I can make to the code so that using document.write in the event handlers works as expected? Can I add another script tag or something else below it so that the parser understand it is still to early to close the document? I am asking because the code in the event handler still gets executed before anything in the following nodes, but the document does get closed before it runs.
The document is not getting closed.  The insertion point is undefined, which is not the same thing: that is the normal state of things most of the time while the document is being parsed, except when directly tokenizing or waiting on an external script.  For example, that's the state of things when we're just waiting on data from the server...

Back to the practical point of view, I can't think of anything, actually.  The insertion point is simply undefined when that event is fired.

Samael, Hsin-Yi is either of you planning to work on this, or should I find someone else?
Flags: needinfo?(sawang)
Flags: needinfo?(htsai)
(In reply to Boris Zbarsky [:bz] from comment #11)
> The document is not getting closed.  The insertion point is undefined, which
> is not the same thing: that is the normal state of things most of the time
> while the document is being parsed, except when directly tokenizing or
> waiting on an external script.  For example, that's the state of things when
> we're just waiting on data from the server...
> 
> Back to the practical point of view, I can't think of anything, actually. 
> The insertion point is simply undefined when that event is fired.
> 
> Samael, Hsin-Yi is either of you planning to work on this, or should I find
> someone else?

Samael is busy with another bug and we were planning that he could take this afterwards, starting in 52 if we weren't too optimistic. :) What timeframe are you looking at here? If we want to see it happening sooner, I could explore other options.
Flags: needinfo?(sawang)
Flags: needinfo?(htsai)
It sounds like this is causing problems out in the real world, so I'd hope for sooner.  I'll take a look, thanks.
Henri, can you take a look?  I'm not really sure who else can review parser/scriptloader interactions.... :(
Attachment #8786600 - Flags: review?(hsivonen)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
(In reply to Boris Zbarsky [:bz] from comment #9)
> So really, we should do the twiddling in OnStreamComplete and then fix the
> onload case separately

Bug 619109 has a patch for that.
Comment on attachment 8786600 [details] [diff] [review]
Make sure the parser insertion point is defined when firing the load event for external <scripts> or firing the error event on a failed external script load (but not other cases, like bogus script URL)

Review of attachment 8786600 [details] [diff] [review]:
-----------------------------------------------------------------

This patch looks good, but I'm marking it as r- in order to request rebasing on top of bug 619109. Sorry about the inconvenience.

::: dom/base/nsScriptElement.cpp
@@ +27,5 @@
>  {
>    if (!aIsInline && NS_FAILED(aResult)) {
> +    nsCOMPtr<nsIParser> parser = do_QueryReferent(mCreatorParser);
> +    if (parser) {
> +      parser->BeginFiringEventWithValidInsertionOnParserInsertedScript();

I suggest rebasing this patch on top of the one from bug 619109. (If it passes try after almost 6 years, I'll request review.)

Doing so should simplify this patch quite a bit main e.g. this change unnecessary.

@@ +74,5 @@
>      event.mFlags.mBubbles = (message != eLoad);
>  
> +    if (parser) {
> +      parser->BeginFiringEventWithValidInsertionOnParserInsertedScript();
> +    }

Bug 619109 simplifies this bit away, too.

::: testing/web-platform/tests/html/semantics/scripting-1/the-script-element/support/script-onload-insertion-point-helper.html
@@ +1,2 @@
> +Some <script src="data:application/javascript,"
> +             onload="document.write('text'); parent.writeDone(document.documentElement.textContent)"></script>

I think it would be prudent to have a genuine http/https external script here in case the data: implementation cheats and becomes available too soon.

Additionally, please extend parser/htmlparser/tests/reftest/bug592656-1.html with the new events to make sure that all the events combine sensibly.
Attachment #8786600 - Flags: review?(hsivonen) → review-
> I suggest rebasing this patch on top of the one from bug 619109.

OK.  So the main difference there is whether before/afterscriptexecute have a defined insertion point.  If we _do_ want that, then we're basically talking comment 9 paragraph 2, which is what the patch in bug 619109 is doing.  I can live with that.  I can rebase on top of that, sure.  It will address the onload case but NOT the onerror case.  So the BeginFiringEventWithValidInsertionOnParserInsertedScript() bit in nsScriptElement::ScriptAvailable will still be needed, as far as I can tell, right?
Flags: needinfo?(hsivonen)
On the other hand, maybe what we should really do is rename the existing boolean and its accessors on nsIParser and call it from both the code bug 619109 touches and the error-event-firing code?
> I suggest rebasing this patch on top of the one from bug 619109. 

Done.  That basically let me remove the changes to nsScriptElement::ScriptEvaluated but nothing else (though we could try to make the onerror bits more generic).


> I think it would be prudent to have a genuine http/https external script here

Done.

> Additionally, please extend parser/htmlparser/tests/reftest/bug592656-1.html 

Done.

        Attachment #8786951 -
        Flags: review?(hsivonen)

    
  

        Attachment #8786600 -
        Attachment is obsolete: true

    
  

        Attachment #8786951 -
        Attachment description: Rebased on top of → Rebased on top of bug 619109

    
  
Depends on: 619109

    
    

    
  
Comment on attachment 8786951 [details] [diff] [review]
Rebased on top of bug 619109

Review of attachment 8786951 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Boris Zbarsky [:bz] from comment #18)
> > I suggest rebasing this patch on top of the one from bug 619109.
> 
> OK.  So the main difference there is whether before/afterscriptexecute have
> a defined insertion point.  If we _do_ want that, then we're basically
> talking comment 9 paragraph 2, which is what the patch in bug 619109 is
> doing.  I can live with that.  I can rebase on top of that, sure.

It seems weird to have the insertion point definedness to back and forth during a sequence of synchronous events. IIRC, beforescriptexecute and afterscriptexecute are Gecko-specific. I guess this is what we get from introducing stuff like that without going through the spec.

(In reply to Boris Zbarsky [:bz] from comment #20)
> Done.

Thank you.

::: parser/htmlparser/nsIParser.h
@@ +232,5 @@
> +     * Call when the event dispatch that was preceded by
> +     * BeginFiringEventWithValidInsertionOnParserInsertedScript completes.
> +     */
> +    virtual void EndFiringEventWithValidInsertionOnParserInsertedScript() = 0;
> +

Adding these as opposed to renaming Begin/EndEvaluatingParserInsertedScript() to be more generic seems a bit of an overkill. I'd prefer to have only one pair of "define/undefine insertion point" methods, but r+ either way.

        Attachment #8786951 -
        Flags: review?(hsivonen) → review+

    
    

    
  
(In reply to Boris Zbarsky [:bz] from comment #18)
> > I suggest rebasing this patch on top of the one from bug 619109.
> 
> OK.  So the main difference there is whether before/afterscriptexecute have
> a defined insertion point.  If we _do_ want that, then we're basically
> talking comment 9 paragraph 2, which is what the patch in bug 619109 is
> doing.  I can live with that.  I can rebase on top of that, sure.  It will
> address the onload case but NOT the onerror case.  So the
> BeginFiringEventWithValidInsertionOnParserInsertedScript() bit in
> nsScriptElement::ScriptAvailable will still be needed, as far as I can tell,
> right?

Yes. I failed to notice that FireScriptAvailable is called from more places than what I was looking at yesterday.
Flags: needinfo?(hsivonen)

    
    

    
  
> renaming Begin/EndEvaluatingParserInsertedScript()

Would be happy to do that.  Want to just call it BeginInsertionPointDefined/EndInsertionPointDefined?  Or PushDefinedInsertionPoint/PopDefinedInsertionPoint?  Or a better name suggestion?

I don't want to do Define/UndefineInsertionPoint because the insertion point may still be defined after the second call...
Flags: needinfo?(hsivonen)

    
    

    
  
(In reply to Boris Zbarsky [:bz] from comment #24)
> Or
> PushDefinedInsertionPoint/PopDefinedInsertionPoint?

Let's pick this name to highlight that the insertion point can still remain defined.
Flags: needinfo?(hsivonen)

    
    

    
  
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c45b331507e2
Make sure the parser insertion point is defined when firing the load event for external <scripts> or firing the error event on a failed external script load (but not other cases, like bogus script URL).  r=hsivonen

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/c45b331507e2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago

          status-firefox51:
          --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Component: DOM → DOM: Core & HTML

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: