Add simple test for opening named windows inside mozbrowser

RESOLVED FIXED in mozilla17

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

(Blocks: 1 bug)

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
I've had this test sitting around for a while, but it was failing intermittently for mysterious reasons.  But I think when I fixed the other mozbrowser oranges, I fixed this one too.  We should check it in.

Patch in a moment.
(Assignee)

Updated

5 years ago
Blocks: 693515
(Assignee)

Comment 1

5 years ago
Created attachment 649168 [details] [diff] [review]
Patch, v1
Attachment #649168 - Flags: review?(mounir)
(Assignee)

Updated

5 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Comment 2

5 years ago
https://hg.mozilla.org/try/rev/cc4749cd5a9f
Attachment #649168 - Flags: review?(mounir) → review+
(Assignee)

Comment 3

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/34a7d2909954
(Assignee)

Updated

5 years ago
Depends on: 773980
Depends on: 780761
Backed out for causing bug 780761 one push after this landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ecf7d9b7580
https://hg.mozilla.org/mozilla-central/rev/34a7d2909954
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
(Assignee)

Comment 6

5 years ago
Except Ed backed this out...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Ed Morley [:edmorley] from comment #4)
> Backed out for causing bug 780761 one push after this landed:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/2ecf7d9b7580

https://hg.mozilla.org/mozilla-central/rev/2ecf7d9b7580
(Assignee)

Updated

5 years ago
Blocks: 780351
(Assignee)

Comment 8

5 years ago
This looks like yet another race condition initializing the message manager during window.open.
(Assignee)

Comment 9

5 years ago
I don't see how this can be happening.

1) We must be hitting nsFrameMessageManager::SendAsyncMessageInternal's check

  if (mAsyncCallback) {
    NS_ENSURE_TRUE(mCallbackData, NS_ERROR_NOT_INITIALIZED);

2) If we're running code in BrowserElementParent.js for an OOP frame, that means nsFrameLoader::ShowRemoteFrame has run and tickled the remote-browser-frame-shown observer:

    mRemoteBrowser->Show(size);
    mRemoteBrowserShown = true;

    EnsureMessageManager();

    nsCOMPtr<nsIObserverService> os = services::GetObserverService();
    if (OwnerIsBrowserFrame() && os && !mRemoteBrowserInitialized) {
      os->NotifyObservers(NS_ISUPPORTS_CAST(nsIFrameLoader*, this),
                          "remote-browser-frame-shown", NULL);
      mRemoteBrowserInitialized = true;
    }

3) So EnsureMessageManager() has been called with mRemoteBrowserShown == true.

4) ... which gives us very few reasons why the message manager's callback data should be null:

nsresult
nsFrameLoader::EnsureMessageManager()
{
  NS_ENSURE_STATE(mOwnerContent);

  nsresult rv = MaybeCreateDocShell();
  if (NS_FAILED(rv)) {
    return rv;
  }

  if (!mIsTopLevelContent && !OwnerIsBrowserFrame() && !mRemoteFrame) {
    return NS_OK;
  }

  if (mMessageManager) {
    if (ShouldUseRemoteProcess()) {
      mMessageManager->SetCallbackData(mRemoteBrowserShown ? this : nullptr);
    }
    return NS_OK;
  }

5) ...unless this message manager is not the message manager which is failing?

Smaug, any pointers here would be appreciated.
(Assignee)

Comment 10

5 years ago
> Smaug, any pointers here would be appreciated.

I think I have a handle on this now...
(Assignee)

Updated

5 years ago
No longer depends on: 773980
(Assignee)

Updated

5 years ago
No longer blocks: 780351
https://hg.mozilla.org/mozilla-central/rev/6c7ed23db4b2
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Depends on: 819320
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.