Closed Bug 428653 Opened 15 years ago Closed 15 years ago

"View MathML source" shows a blank window

Categories

(Toolkit :: View Source, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9

People

(Reporter: gribozavr, Assigned: Gavin)

References

()

Details

(Keywords: regression, useless-UI)

Attachments

(1 file, 2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; ru; rv:1.9pre) Gecko/2008041213 Firefox/3.0pre
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; ru; rv:1.9pre) Gecko/2008041213 Firefox/3.0pre

"View MathML source" menu item from the formula's context menu opens a blank window.


Reproducible: Always

Steps to Reproduce:
1. Open http://www.mozilla.org/projects/mathml/demo/basics.xhtml
2. Right click any formula and choose "View MathML source" from the menu

Actual Results:  
A blank window opens, no MathML source there.

Expected Results:  
A window with source should open.

It works fine in http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2008/01/2008-01-25-04-trunk/firefox-3.0b3pre.en-US.linux-i686.tar.bz2

It shows a blank window starting from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2008/01/2008-01-26-04-trunk/firefox-3.0b3pre.en-US.linux-i686.tar.bz2 and up to the latest trunk.
The same happens on Firefox 3 Beta 5 on Windows XP, too.
Error: uncaught exception: [Exception... "Security error"  code: "1000" nsresult: "0x805303e8 (NS_ERROR_DOM_SECURITY_ERR)"  location: "<unknown>"]

I don't know how the MathML source stuff works, but given that it's the same error message as the complaints about regressions starting in bug 397791 comment 37, and the same day, I guess it's tripping over the same thing.

Drivers: yeah, I realize MathML viewsource isn't a blocker in and of itself, but shipping useless-UI like this probably is, and if it's not fixable at this point we should at least remove the menuitem until it works.
Blocks: 397791
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-firefox3?
OS: Linux → All
Hardware: PC → All
Version: unspecified → Trunk
Attached patch The nuclear option (obsolete) — Splinter Review
I was wavering around trying to decide when would be the right time to deploy the nuclear option, when I realized that if we get a patch to fix it, backing out the comment-out is simple, so there's no reason not to take out the useless-UI now, before it slips through the cracks.
Attachment #315344 - Flags: review?(gavin.sharp)
Attached patch patch? (obsolete) — Splinter Review
This seems to fix it. Not sure why it's required.
Looking at the first hunk of attachment 298836 [details] [diff] [review], I can see why that patch broke this - it required the caller to have the same principal as the document when calling document.open(), which isn't the case here. I don't understand why my patch fixes it, though, since as far as I can tell there are still two different principals involved (the "content" document and the chrome code). I don't understand why the wrapper has an effect. jst/sicking can you explain?
Comment on attachment 315344 [details] [diff] [review]
The nuclear option

Let's hold off on this since we might be able to fix it.
Attachment #315344 - Flags: review?(gavin.sharp)
Comment on attachment 315344 [details] [diff] [review]
The nuclear option

view source is a pretty important part of the web, so I don't think I like the nuclear option
Attachment #315344 - Flags: ui-review-
jst/jonas: this is blocking now, can you help gavin out with his question in comment 5?
Assignee: nobody → gavin.sharp
Flags: blocking-firefox3? → blocking-firefox3+
Yeah, view source is pretty important, and it still works. View the DOM created by source is pretty important, too, and it still works - view selection source works fine, whether what's selected is MathML or not. What doesn't work is viewing the DOM created by MathML source by right-clicking anywhere within the rendered MathML without having to select it.

But, I'm all for the humor of saying that even though for quite a while in bug 324857 we weren't willing to block 1.9 on having MathML even render, we're now willing to block Firefox 3 even if the only remaining issue is that MathML authors trying to figure out what we're doing with their MathML have to select a formula in order to see the DOM source.
jst or blake really is the authority here, but here's how I understand it:

When you use the XPCNativeWrapper (which is what you'll have before the patch) we never change what principal we make the call with, we simply make sure that you never call any content set functions but instead go directly to the browser implementation.

When you call using the SafeJSObjectWrapper we will use the principal of the object to actually make the call, which is likely why it succeeds here. This was done to make sure that content-overridden functions got called with content level security contexts. I guess an interesting side effect is that even native functions will execute with content level access.

Unfortunately I don't know if relying on this behavior is a good idea or not...

Yes, Jonas' explanation is right here. Whether we should rely on this or not is a good question. I don't see this ever changing, but if we want this code to be independent of whether that does change or not, this code could use evalInSandbox() to evaluate the code that does the document.open etc here in the context of the webpage. I don't know if I care strongly either way, but a mochitest that makes sure we don't break this again would be golden!
Yeah, I agree with jst, if we can get a test for this I think it's fine to check in the patch as is.
Attached patch patch with testSplinter Review
Attachment #315344 - Attachment is obsolete: true
Attachment #315347 - Attachment is obsolete: true
Attachment #316280 - Flags: superreview?(jst)
Attachment #316280 - Flags: review?(mano)
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → Firefox 3
Whiteboard: [has patch][needs review mano]
Attachment #316280 - Flags: superreview?(jst) → superreview+
Comment on attachment 316280 [details] [diff] [review]
patch with test

r=mano
Attachment #316280 - Flags: review?(mano) → review+
Keywords: checkin-needed
Whiteboard: [has patch][needs review mano] → [has patch]
Attachment #316280 - Flags: approval1.9?
Keywords: checkin-needed
Whiteboard: [has patch] → [has patch][needs approval]
Comment on attachment 316280 [details] [diff] [review]
patch with test

a1.9=beltzner
Attachment #316280 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Whiteboard: [has patch][needs approval]
mozilla/toolkit/components/viewsource/Makefile.in 	1.3
mozilla/toolkit/components/viewsource/content/viewPartialSource.js 	1.15
mozilla/toolkit/components/viewsource/test/Makefile.in 	1.1
mozilla/toolkit/components/viewsource/test/test_428653.xul 	1.1 
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050804 Minefield/3.0pre. 
Status: RESOLVED → VERIFIED
Product: Firefox → Toolkit
Ugh.  So all of this code was actually relying on a bug in document.write, and it breaks if I fix that (bug 465806).
I guess that was discussed earlier in this bug too.  It's a good thing that we had a test for this, indeed.  ;)

So why are we using document.write instead of a data: URL, say?  And if we are, can we do it indirectly either via evalInSandbox as suggested above?
er, "either via evalInSandbox as suggested above or via setTimeout on the target window or something".
(In reply to comment #19)
> So why are we using document.write instead of a data: URL, say?

I don't see any reason why a data: URL wouldn't work.
OK.  I switched this to a data: URL in bug 465806 and updated the test to test that document.write throws a security error.
The mochitest fails with my current iteration of parserless about:blank (attachment 541670 [details] [diff] [review] [review]) applied. How do I need to prepare the generated about:blank to make sure the .open() call on it throws a security exception as expected? See also bug 601803 comment 37.
> How do I need to prepare the generated about:blank to make sure the .open()
> call on it throws a security exception as expected?

You need to give it the correct principal.  What principal are you giving your about:blank right now?  Note that this discussion is really pertinent to bug 465806, which is what made the security exceptions start happening.
For posterity: bz figured this out in IRC. If the docshell isn't of type chrome and the principal the initial about:blank is about to get is the system principal, it needs to get null instead.
You need to log in before you can comment on or make changes to this bug.