Closed Bug 437361 Opened 16 years ago Closed 16 years ago

Pass PR_TRUE for aDoJSFixups in nsGlobalWindow::ShowModalDialog

Categories

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

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: Waldo, Assigned: mozilla+ben)

Details

Attachments

(2 files, 2 obsolete files)

See bug 389988 comment 2.

I looked into this awhile ago and also didn't see any problems that could arise, but the code's changed a lot since then.  A double-check would be good, and it'd be a good way to start understanding a little Mozilla code for anyone interested in doing so.
Whiteboard: [good first bug]
When this is fixed, I think the patch that was landed for bug 389988 should be backed out, since it could hide exceptions that might actually matter to the script.
Flags: wanted1.9.1?
Flags: wanted1.9.1? → wanted1.9.1+
Priority: -- → P3
Version: unspecified → Trunk
Reassigning to Benjamin Newman who'll take a look at this one.
Assignee: nobody → bnewman
I see this precondition in nsGlobalWindow::OpenInternal:

  NS_PRECONDITION(!aDoJSFixups || !aCalledNoScript,
                  "JS fixups should not be done when called noscript");

I understand the intuition that JS fixups should be unnecessary if scripts are disallowed in the opened window (which means more precisely that the window can't examine the JS context on the stack), but I don't see why this should be a hard precondition.  Moreover, this bug is all about fixing up JS problems with the opening of the window (dialog) that have nothing to do with whether scripts are allowed in the window, so the conjunction of aDoJSFixups && aCalledNoScript seems safe.

In short, I have a patch and test case ready, but I'm not sure what to do about the precondition.  Kill it? Live with the warning?  Pass PR_FALSE for aCalledNoScript?
Attached patch proposed patch with test case (obsolete) — Splinter Review
Not looking for a formal review just yet, just wanted to make my previous comment a little more concrete.
So... aCalledNoScript is used for two things:

1)  Deciding whether to look at the JS stack when doing the window name lookup.  I assume that we DO want to do that for showModalDialog?

2)  Deciding how to call into the window watcher.  Not sure what we want there; jst might know offhand.

Depending on the answers to those questions, the right answer is either to kill the assertion, pass PR_FALSE for aCalledNoScript, or split it up into two separate boolean arguments.
I think we just want to kill the assertion in this case passing in PR_TRUE for aDoJSFixups and aCalledNoScript does exactly what we want. We don't want to pass false for aCalledNoScript as that would make the window watcher handle our arguments differently, and incorrectly, and I don't see the need to split this out into yet another argument here. So I'd say kill the assertion.
This test

1. attempts to open a modal dialog with dom.disable_open_during_load set to true, which fails but should not throw an exception, and

2. attempts to open a modal dialog with url 'about:config', which *should* throw an exception, since opening about:config is forbidden for security reasons (note that dom.disable_open_during_load is set to false to avoid interference from the popup blocker).

Before my #437361 bug fix (forthcoming), the first test would pass at the expense of the second; that is, in order to suppress the exception resulting from the popup blocker blocking a modal dialog, we suppressed all (some potentially useful) javascript exceptions.  With my patch, the dialog is blocked cleanly in the first case but an appropriate exception is thrown in the second, so both tests pass.

In order to manipulate user preferences reliably in the presence of exceptions, I wrote a small JavaScript library (mozprefs.js).  Once these tests are reviewed/landed, I'll rewrite the following section of the MochiTest wiki page:

http://developer.mozilla.org/en/docs/Mochitest_FAQ#What_if_I_need_to_change_a_preference_to_run_my_test.3F
Attachment #330813 - Flags: review?(bzbarsky)
Removing the precondition, in agreement with jst's comment.
Attachment #330315 - Attachment is obsolete: true
Attachment #330814 - Flags: superreview?
Attachment #330814 - Flags: review?(bzbarsky)
Comment on attachment 330814 [details] [diff] [review]
actual proposed patch

r+sr=bzbarsky
Attachment #330814 - Flags: superreview?
Attachment #330814 - Flags: superreview+
Attachment #330814 - Flags: review?(bzbarsky)
Attachment #330814 - Flags: review+
Comment on attachment 330813 [details] [diff] [review]
regression tests (using my javascript preferences wrapper)

Add a test that the showModalDialog with the returnvalue actually works if popups are allowed, and looks great.
Attachment #330813 - Flags: review?(bzbarsky) → review+
Attachment #330813 - Attachment is obsolete: true
Attachment #330818 - Flags: superreview?(bzbarsky)
Comment on attachment 330818 [details] [diff] [review]
test cases (including returnValue check)

looks good
Attachment #330818 - Flags: superreview?(bzbarsky) → superreview+
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Checked in.  Not sure how easy this would be to test....
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [good first bug]
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: