Closed Bug 585620 Opened 15 years ago Closed 15 years ago

Twitter Widget replaces this page (uses document.write too late?)

Categories

(Core :: DOM: HTML Parser, defect, P3)

Product:
Core
Component:
DOM: HTML Parser
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: ryan_pal, Assigned: hsivonen)

References

(
URL
)

Details

User-Agent: Mozilla/5.0 (Windows; Windows NT 6.1; rv:2.0b2) Gecko/20100720 Firefox/4.0b2 Build Identifier: Mozilla/5.0 (Windows; Windows NT 6.1; rv:2.0b2) Gecko/20100720 Firefox/4.0b2 When you open a site containing "Twitter widget", a blank page is opened containing the widget. Reproducible: Always Steps to Reproduce: 1.Open the site. 2.Wait for the page to load. Actual Results: Once the page is loaded, it advances to a blank page containing the Twitter widget Expected Results: Twitter widget should correctly display within the same page and should not open (or redirect to) a blank page This flaw could be exploited by some "malicious" widgets.
I can see the issue you mentioned using Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; rv:2.0b3) Gecko/20100805 Firefox/4.0b3. Will leave as a security bug until the triage team can review tomorrow.
It is widely known that malicious widgets can replace the top-level page, so this bug doesn't need to hidden. But why is the Twitter widget causing this? CCing Henri, who has been tweaking the document.write add-vs-replace heuristics for the last few months.
Group: core-security
Summary: Problem with Twitter Widget on HTML pages → Twitter Widget replaces this page (uses document.write too late?)
Should be fixed in the latest trunk build.
Works for me on Mac. Reporter, do you still see this in the latest nigthly or beta?
still not fixed as of 4.0b4 (Win32).
Oops. I'm seeing this on Mac after all. Also on Windows and Linux. I'll investigate.
Status: UNCONFIRMED → NEW
Component: General → HTML: Parser
Ever confirmed: true
Priority: -- → P3
Product: Firefox → Core
QA Contact: general → parser
Version: unspecified → Trunk
OS: Windows 7 → All
Hardware: x86 → All
This appears to be timing-dependent. I can see this in opt builds but I can't observe the problem in debug builds. I emailed the author of the widget.
Assignee: nobody → english-us
Status: NEW → ASSIGNED
Component: HTML: Parser → English US
Priority: P3 → --
Product: Core → Tech Evangelism
QA Contact: parser → english-us
Version: Trunk → unspecified
Henri, why do you think this is tech evangelism? It seems to work just fine in IE8, Opera, Chrome and Firefox3.6.8.
(In reply to comment #8) > Henri, why do you think this is tech evangelism? I had a reason to believe the widget was doing a write from a timeout. However, I was unable to verify that belief because the problem is timing-dependent and didn't show up in a debug build. After getting an email reply from the author of the widget code, I no longer consider this evang--at least not until I investigate more. > It seems to work just fine in IE8, Opera, Chrome and Firefox3.6.8. I'd love to know if the "Join the conversation" part is displayed deterministically in IE8. Working in Chrome or Firefox releases isn't surprising. However, it would be interesting to know if the page always works in WebKit/Chromium with their HTML5 parser (including the tree builder--not just their tokenizer) enabled.
Assignee: english-us → nobody
Component: English US → HTML: Parser
Priority: -- → P3
Product: Tech Evangelism → Core
QA Contact: english-us → parser
Version: unspecified → Trunk
Assignee: nobody → hsivonen
Oh, and the widget runs different code paths in IE, so the page working doesn't prove that the code that's run in Firefox 4 beta would work in IE8.
(In reply to comment #9) > (In reply to comment #8) > > Henri, why do you think this is tech evangelism? > > I had a reason to believe the widget was doing a write from a timeout. However, > I was unable to verify that belief because the problem is timing-dependent and > didn't show up in a debug build. After getting an email reply from the author > of the widget code, I no longer consider this evang--at least not until I > investigate more. > > > It seems to work just fine in IE8, Opera, Chrome and Firefox3.6.8. > > I'd love to know if the "Join the conversation" part is displayed > deterministically in IE8. Working in Chrome or Firefox releases isn't > surprising. However, it would be interesting to know if the page always works > in WebKit/Chromium with their HTML5 parser (including the tree builder--not > just their tokenizer) enabled. Tested it on IE8 and the "Join the conversation" widget displays correctly at the bottom of the page.
The write is called when an external script is evaluated. This is not about timeouts. Also, when loading the page in Chrome, Web Inspector suggests there are two instances of the Twitter widget on the page. There are two JSONP loads.
(In reply to comment #12) > Also, when loading the page in Chrome, Web Inspector suggests there are two > instances of the Twitter widget on the page. There are two JSONP loads. Whoa, both are visible, even. I wonder how I managed to miss that earlier.
(In reply to comment #13) > (In reply to comment #12) > > Also, when loading the page in Chrome, Web Inspector suggests there are two > > instances of the Twitter widget on the page. There are two JSONP loads. > > Whoa, both are visible, even. I wonder how I managed to miss that earlier. Strange behaviour though for Firefox, but why load the widget in a blank page and not on the parent window or _self?
So here's what happens: 1) The page first loads the twitter widget as designed. 2) Then later, the page gets jQuery(...).html() of the widget. 3) It takes that value (and other stuff) and passes it to jQuery(...).html() of another element. 4) jQuery performs its own HTML fragment parsing and, unlike innerHTML, runs the scripts in the fragment. Thus, jQuery tries to rerun both the external widget library and the internal widget initialization script. 5) jQuery inserts a new script element with its .src set to the URL of the twitter widget library into the head element of the document. 6) Immediately afterwards, jQuery inserts a new script whose textContent is a copy of the widget initialization script into head. This script doesn't run immediately, because there's now a pending external script in the script loader. 7) Now the parser-inserted script that called jQuery finishes running and the parser is no longer in a state that accepts document.write(). 8) Later, the external twitter widget library loads and is evaluated without trouble. 9) Immediately afterwards, the script-inserted inline script that got blocked by the pending script-inserted external script gets evaluated, too. This evaluation calls into the twitter widget library, which calls document.write(). Since there's now no parser-inserted script executing, the parser refuses the document.write(). Since there's now also no external script executing, the document isn't write-neutralized, so a call to document.open() is implied.
That the script-inserted inline script doesn't run immediately at step #5 but runs in step #9 is not HTML5-compliant. We should either change Gecko to run the inline script at step #5 even though a pending external script exists--making Gecko HTML5-compliant on this point. Or we should make the parser block with in step #7 and leave document.write() permitted--and change the HTML5 spec accordingly. However, this would make the behavior of event handlers racy, since event handlers could write into the parser while the parser is waiting for the script-inserted external script to load. sicking, do you have an opinion? I'd be inclined to make script-inserted inline script not wait for pending script-inserted external scripts (which is what the spec says).
http://hsivonen.iki.fi/test/moz/script-inserted-script.html HTML5 spec: internal, external Trident (both IE8 and IE9) and WebKit (both old and new parser): internal, external Gecko and Presto: external, internal So the spec aligns with Trident and WebKit.
Making script-inserted inline scripts always execute immediately indeed fixes this bug. The problem is that when the particular script in question here needs to run, nsContentUtils::IsSafeToRunScript() returns false. I haven't investigated yet, why that is. Sure seems odd considering that the script that is inserting the script-inserted script got the chance to run.
Nominating as a blocker, because without HTML5-compliant execution of script-inserted inline scripts but with other HTML5-isms in place, authors can use jQuery to shoot themselves in the foot in a way that leads to bad UX in Firefox but not in IE/WebKit.
blocking2.0: --- → ?
can this fix be implemented at least as a temporary workaround in the next build?
(In reply to comment #16) > sicking, do you have an opinion? I'd be inclined to make script-inserted inline > script not wait for pending script-inserted external scripts (which is what the > spec says). Indeed, this is what I think we should do. The only reason we didn't do this for last release was because we found out too late in release cycle. Henri, would you be able to fix that for next beta? I think we might have a separate bug on this, but I'm not sure.
In general, all DOM inserted scripts internal scripts should execute right away, and all DOM inserted external scripts should act as if they have .async=true
(In reply to comment #21) > Henri, would you be able to fix that for next beta? Yes. (I think I figured out the nsContentUtils::IsSafeToRunScript() thing already. Script-inserted inline scripts want to run as scriptrunners.)
This bug now depends on a blocker bug, so no need to make this one block.
blocking2.0: ? → ---
The dependency landed http://hg.mozilla.org/mozilla-central/rev/0ab712643a66
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.