Closed Bug 1384153 Opened 7 years ago Closed 7 years ago

Artifact and local builds crashing content tabs on latest autoland to m-c merge

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: Mardak, Assigned: haik)

Details

(Keywords: dogfood)

Attachments

(1 file)

This merge commit is empty and is just merging two parents:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=80394cbcae0f02da8eb801edd92e56f974e5db4f

The artifact build for that is broken. All content tabs seem to crash immediately, so about:home, about:newtab, any content pages. Non-content pages, e.g., about:, about:config, load fine.

Its two parents are both successfully building with artifact builds:
servo: Merge #17846 - Allow nonstandard font-weight values for system fonts (from Manishearth:font-weight); r=upsuper a=tomcat
Backed out changeset a7da25c837d8 (bug 1372689) for failing browser-chrome's browser_startup_images.js about toolbarbutton-dropdown-arrow.png. r=backout
I suppose just to confirm, I did build locally without artifact for 80394cbcae0f merge autoland to mozilla-central a=merge

It ran fine.
Summary: Artifact builds broken crashing content tabs on latest autoland to m-c merge → Artifact builds broken crashing content tabs on latest autoland to m-c merge (80394cbcae0f02da8eb801edd92e56f974e5db4f)
I'm able to reproduce tab crashes from an artifact build at this revision on macOS but not Linux. The stacks produced point to bug 1380690.
Keywords: dogfood
Assignee: nobody → haftandilian
I'm working on a fix for this and, if I don't hit any surprises, should have it posted within a few hours.
I can see that we are crashing in the artifact build due to the call to CFStringGetCStringPtr() in GetStringValueFromBundlePlist() returning NULL. CFStringGetCStringPtr() is documented as returning NULL if a CString can't be immediately returned efficiently and the code does not handle that case properly. The fix for that is to then call CFStringGetCString(). The posted fix does this.

But I haven't been able to verify the fix addresses the problem because I haven't been able to generate artifact packages for a local build or change pushed to try. Any pointers on how to do that would be helpful.
(In reply to Haik Aftandilian [:haik] from comment #6)
> I can see that we are crashing in the artifact build due to the call to
> CFStringGetCStringPtr() in GetStringValueFromBundlePlist() returning NULL.
> CFStringGetCStringPtr() is documented as returning NULL if a CString can't
> be immediately returned efficiently and the code does not handle that case
> properly. The fix for that is to then call CFStringGetCString(). The posted
> fix does this.
> 
> But I haven't been able to verify the fix addresses the problem because I
> haven't been able to generate artifact packages for a local build or change
> pushed to try. Any pointers on how to do that would be helpful.

Pulling a try push so it's present in your local repo and then setting MOZ_ARTIFACT_REVISION to the try push's push head will force |./mach build| to pick up artifacts from that push.

Doing this on a try push with that patch fixes the startup crash for me locally.
Comment on attachment 8890752 [details]
Bug 1384153 - Artifact and local builds crashing content tabs on latest autoland to m-c merge.

https://reviewboard.mozilla.org/r/161946/#review167402

This also fixes local builds.

::: security/sandbox/common/SandboxSettings.cpp:87
(Diff revision 1)
> +    return NS_ERROR_FAILURE;
> +  }
>  
> +  aValue.Assign(valueBuffer);
> +  free(valueBuffer);
> +	

nit: extra whitespace
Attachment #8890752 - Flags: review+
Summary: Artifact builds broken crashing content tabs on latest autoland to m-c merge (80394cbcae0f02da8eb801edd92e56f974e5db4f) → Artifact and local builds crashing content tabs on latest autoland to m-c merge
Comment on attachment 8890752 [details]
Bug 1384153 - Artifact and local builds crashing content tabs on latest autoland to m-c merge.

https://reviewboard.mozilla.org/r/161946/#review167402

> nit: extra whitespace

Fixed the whitespace and added error checking on the key string and value string length.
Pushed by haftandilian@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d5d0c120032d
Artifact and local builds crashing content tabs on latest autoland to m-c merge. r=spohl
https://hg.mozilla.org/mozilla-central/rev/d5d0c120032d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.