Closed Bug 707749 Opened 13 years ago Closed 12 years ago

I need to click on the bookmarklet twice to make it work. If document.write is included in the bookmarklet (involves navigator reparenting)

Categories

(Core :: DOM: Navigation, defect)

11 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla12
Tracking Status
firefox11 + verified

People

(Reporter: alice0775, Assigned: mrbkap)

References

Details

(Keywords: regression, Whiteboard: [qa!])

Attachments

(1 file)

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/1bd7482ad4d1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111205 Firefox/11.0a1 ID:20111205031110

I need to click on the bookmarklet twice to make it work. If document.write is included in the bookmarklet.


Reproducible: Always

Steps to Reproduce:
1. Start Firefox with clean profile
2. Bookmark javascript:document.write("aaaaaaa");document.close();
3. Open any web page (ex. http://www.mozilla.org/en-US/about/ )
4. Click the bookmarklet created in step2

5. Click again the bookmarklet created in step2


Actual Results:
  *Nothing happen at Step4, and And an error message is shown in Error console.

  Error: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED)
  [nsIDOMHTMLDocument.write]
  Source file: javascript:document.write("aaaaaaa");document.close();
  Line: 1

  *However successfully executed at Step5.

Expected Results:
  The bookmarklet should be executed at Step4 as well as Step5.


Regression window(m-c)
Works:
http://hg.mozilla.org/mozilla-central/rev/9740118b9dcc
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111203 Firefox/11.0a1 ID:20111203031109
Fails:
http://hg.mozilla.org/mozilla-central/rev/a68c96c1d8e0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111203 Firefox/11.0a1 ID:20111203031117
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9740118b9dcc&tochange=a68c96c1d8e0


Regression window(m-i)
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/8304db7e46bb
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111202 Firefox/11.0a1 ID:20111202212018
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2cddf1a5d9e2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111202 Firefox/11.0a1 ID:20111202221915
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8304db7e46bb&tochange=2cddf1a5d9e2

Suspected: Bug 700202
First report is in mozillazine( http://forums.mozillazine.org/viewtopic.php?p=11531009#p11531009 )
This might indeed be a regression from sfink's patch. But since this patch just makes us propagate exceptions correctly, it's likely that it's just exposing a latent bug. sfink, want to take a look?
This problem affects not only execution of bookmarklt but also execution  of web page Javascript.

After I failed to execute the bookmarklet( i.e. I click the bookmarklet once) on http://www.mozilla.org/projects/firefox/prerelease.html. 
Errors appear in error console when mouse hover web page.

Error: attempt to run compile-and-go script on a cleared scope
Source file: chrome://global/content/bindings/videocontrols.xml
Line: 454

Error: attempt to run compile-and-go script on a cleared scope
Source file: http://mozcom-cdn.mozilla.net/org/script/1.0/jquery-1.4.3.min.js
Line: 56

etc...

And on https://docs.google.com/
Errors appear in error console when mouse hover web page.
And checkboxes no longer work until I load the page again.


Error: attempt to run compile-and-go script on a cleared scope
Source file: https://docs.google.com/static/doclist/client/js/4058151398-doclist_modularized-gecko_core.js
Line: 181
(In reply to Bobby Holley (:bholley) from comment #2)
> This might indeed be a regression from sfink's patch. But since this patch
> just makes us propagate exceptions correctly, it's likely that it's just
> exposing a latent bug. sfink, want to take a look?

What I'm seeing is in dom/base/nsDOMClassInfo.cpp:

  nsGlobalWindow *win = static_cast<nsGlobalWindow*>(nav->GetWindow());
  if (!win) {
    NS_WARNING("Refusing to create a navigator in the wrong scope");

    return NS_ERROR_UNEXPECTED;
  }

that error is getting caught, and now nsXPConnect::MoveWrappers() no longer ignores it.

I see bugs@pettay.fi is CC'd. smaug, what's the right way to handle this?

There's also the cleared scope error. I'm not sure whether it's breaking anything or not. It's triggered by a script in the page you started from (in my case, some horribly mangled URL from www.google.com, being invoked on an HTMLInputElement due to a blur event, but Alice0775 has other examples in comment 3.) It's probably something about the ordering of actions when using document.write from a bookmarklet. Tracing through with the debugger, though, it seems like this error will be ignored and so is probably irrelevant to the behavior change in this bug. (It ends setting a flag on the event saying that it provoked an exception, but that's it.)
I would probably back out patches for bug 700202, and try to fix it so that this bug doesn't regress.
(and add an automated testcase for this)
Did bug 690952 change some behavior here?
I think 
nsGlobalWindow *win = static_cast<nsGlobalWindow*>(nav->GetWindow());
was added in that bug.
My _guess_ is that something like this happens:
inner window A is created, and Navigator for it, but not js wrapper for Navigator.
Then document.write destroys A and creates a new inner window B, and for some reason after that
the Navigator for A gets it js wrapper created.
(In reply to Olli Pettay [:smaug] from comment #5)
> So what part of https://bugzilla.mozilla.org/show_bug.cgi?id=700202 is
> causing the problem?

This part:

     nsCOMPtr<nsIScriptGlobalObject> newScope(do_QueryReferent(mScopeObject));
     if (oldScope && newScope != oldScope) {
-      nsContentUtils::ReparentContentWrappersInScope(cx, oldScope, newScope);
+      rv = nsContentUtils::ReparentContentWrappersInScope(cx, oldScope, newScope);
+      NS_ENSURE_SUCCESS(rv, rv);
     }
   }

Previously, ReparentContentWrappersInScope would fail but the error would be ignored. Now it aborts out of nsHTMLDocument::Open.

We could back out bug 700202 to fix this bug temporarily if the real problem is too hard to fix for now. Obviously, it would be better to prevent ReparentContentWrappersInScope() from failing, or at least detect and suppress the error closer to its source when it is known to be ignorable. (For correctness, the exception needs to be cleared on the JSContext if it's going to be ignored. Otherwise it can interfere with the next JS code run with that context.)
(In reply to Steve Fink [:sfink] from comment #10)
> We could back out bug 700202 to fix this bug temporarily if the real problem
> is too hard to fix for now.

This could break bookmarklets and affects 11, which is now in Aurora, so I'd like to get it fixed promptly. Do you (and smaug) think it can be fixed within a few weeks, or would you prefer to back out bug 700202?
So did anyone check why and where is the wrong navigator object created?
Is comment 9 right?
Summary: I need to click on the bookmarklet twice to make it work. If document.write is included in the bookmarklet → I need to click on the bookmarklet twice to make it work. If document.write is included in the bookmarklet (involves navigator repainting)
Summary: I need to click on the bookmarklet twice to make it work. If document.write is included in the bookmarklet (involves navigator repainting) → I need to click on the bookmarklet twice to make it work. If document.write is included in the bookmarklet (involves navigator reparenting)
Attached patch Proposed fixSplinter Review
So, the problem is that we've called FreeInnerObjects by the time the document.write reparenting happens. FreeInnerObjects invalidates the navigator, meaning that we can't even call PreCreate on it without throwing. The easiest fix is to move the navigator object explicitly in SetNewDocument so that it continues to work. I think that given the semantics of document.open, this is correct.
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #584052 - Flags: review?(jst)
Oh, the other key is that navigator has to already have been created when document.open is called.
Blake, does the patch fix also bug 713002?
(In reply to Olli Pettay [:smaug] from comment #16)
> Blake, does the patch fix also bug 713002?

Sorry, I'm not Blake.
I've just tried the proposed patch (attachment#584052 [details] [diff] [review]) to the source 
http://hg.mozilla.org/mozilla-central/rev/798b00a6fe29 locally on Ubuntu.

Things looking good about both of bug 707749#c0 and bug 713002#c0 .
Comment on attachment 584052 [details] [diff] [review]
Proposed fix

Looks good, r=jst
Attachment #584052 - Flags: review?(jst) → review+
Component: JavaScript Engine → Document Navigation
QA Contact: general → docshell
https://bugzilla.mozilla.org/show_bug.cgi?id=707749
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Comment on attachment 584052 [details] [diff] [review]
Proposed fix

This is needed to fix the regression on bookmarklets in Aurora.
Attachment #584052 - Flags: approval-mozilla-aurora?
Comment on attachment 584052 [details] [diff] [review]
Proposed fix

[triage comment]
Approved for Aurora. JS regression in Fx11 that affects web content and bookmarklets.
Attachment #584052 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Mozilla/5.0 (Windows NT 5.1; rv:12.0a1) Gecko/20111230 Firefox/12.0a1 ID:20111230031151
Status: RESOLVED → VERIFIED
Whiteboard: [qa+]
Bookmarklet is opened after the first click, no errors in Error Console.

Verified fixed on Firefox 11b4:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0) Gecko/20100101 Firefox/11.0
Mozilla/5.0 (X11; Linux i686; rv:11.0) Gecko/20100101 Firefox/11.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0) Gecko/20100101 Firefox/11.0
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.