Closed Bug 1340747 Opened 7 years ago Closed 7 years ago

TryRemoteBrowser can run too late if it's not safe to run scripts when instantiating a <xul:browser>

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: nika)

References

Details

Attachments

(1 file)

See this code: <http://searchfox.org/mozilla-central/rev/cac6cb6a10afb8ebb2ecfbeeedaff7c66f57dd75/dom/base/nsDocument.cpp#7073>

If it's safe to run scripts, this runs the frameloader initialization immediately, otherwise it defers it all until a time when running scripts is safe.

Bug 1304492 however added this code: <https://hg.mozilla.org/mozilla-central/rev/19e3f7722664#l1.19>.  The .loadContext getter fails if the remote browser has not been setup yet.  If running scripts isn't safe above, setting up the remote browser is deferred since it happens as part of frameloader initialization.  Therefore if that happens, the getter returns null and that code throws.

Mossop was hitting this in bug 1337627.

As far as I can tell, nothing in TryRemoteBrowser actually requires a script runner, perhaps maybe up to here: <http://searchfox.org/mozilla-central/rev/59cd73fbfa14384a81a4e3eb17d3881b372702c4/dom/base/nsFrameLoader.cpp#2964>.  So we should be able to run that part immediately and defer the rest to happen under a script runner.

Boris, can you please double check my understanding of this above?

Michael, since you've been enjoying hacking on frameloader stuff, do you mind taking this one please?  ;-)

Thanks!
Flags: needinfo?(michael)
Flags: needinfo?(bzbarsky)
Summary: TryRemoteBrowser can fail if it's not safe to run scripts when instantiating a <xul:browser> → TryRemoteBrowser can run too late if it's not safe to run scripts when instantiating a <xul:browser>
> The .loadContext getter fails if the remote browser has not been setup yet.

If we're at the point where that getter runs, we are running script.  I would expect the scriptrunner has run by then, no?  If not, why not?

In terms of what's safe to do sync... I don't know offhand.  I'd have to dig into the details of everything that happens in the frameloader code there, because we fire random events from various places, etc... :(
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #1)
> > The .loadContext getter fails if the remote browser has not been setup yet.
> 
> If we're at the point where that getter runs, we are running script.  I
> would expect the scriptrunner has run by then, no?  If not, why not?

I'm not quite sure how this happens, since I was asking Mossop to debug this over IRC.  There may have been a script blocker on the stack when initializing the frameloader.  Note that the getter runs from the constructor of the <browser remote> binding, but LoadSrc runs when the XUL element is bound to tree.
This isn't a full patch which does the full amount of work you were suggesting, but it fixes the problem which you were running into in bug 1337627. I did a try push with the patch enabled and it seemed to look pretty good:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f33c05342578ce815f051a0a6ae42957197eafb

Effectively in the old code, in the non-remote codepath if the browser hadn't been initialized yet the code would go through the effort to create the DocShell when the loadContext was requested, while in the remote case, this wouldn't occur. This change makes us `TryRemoteBrowser()` when the loadContext is requested, if we don't have one already.

This should be safe because at the point in time when the loadContext would be being requested, the call to TryRemoteBrowser is already effectively queued up. It has been queued up to run when scripts are allowed to run again, and just happened to be slotted in after the script runner which is trying to access the loadContext would be as far as I can tell.

MozReview-Commit-ID: GhCNwG0uMGb
Attachment #8840963 - Flags: review?(ehsan)
Flags: needinfo?(michael)
Assignee: nobody → michael
Comment on attachment 8840963 [details] [diff] [review]
Try to initialize the remote browser when getting the frameloader's loadContext

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

Nice fix!  This way we don't have to answer the hard questions before.  Not sure why I didn't think of this way myself.  :-)
Attachment #8840963 - Flags: review?(ehsan) → review+
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a932a0a9cffe
Try to initialize the remote browser when getting the frameloader's loadContext, r=ehsan
https://hg.mozilla.org/mozilla-central/rev/a932a0a9cffe
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1363240
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.